Fix FileResponse fallback code

[mdellweg]

In case the FileResponse is using _sendfile_fallback and the requested
range is smaller then the chunk size, we need to only read and send
count bytes.

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new news fragment into the CHANGES folder
  • name it <issue_id>.<type> for example (588.bugfix)
  • if you don't have an issue_id change it to the pr id after creating the pr
  • ensure type is one of the following:
    • .feature: Signifying a new feature.
    • .bugfix: Signifying a bug fix.
    • .doc: Signifying a documentation improvement.
    • .removal: Signifying a deprecation or removal of public API.
    • .misc: A ticket has been closed, but it is not of interest to users.
  • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

[DavidRomanovizc]

Hi, @mdellweg, this issue is still relevant?

[mdellweg]

I would guess so.

[DavidRomanovizc]

@mdellweg, Since there has been no activity over the past year can you send an code example in which such a issue occurs. Also you need fill CONTRIBUTORS.txt.and change unit test.

[Dreamsorcerer]

As mentioned, this still needs a regression test. It should go in test_sendfile_functional.py and use the sender fixture to show that the behaviour is the same with/without the fallback.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.77%. Comparing base (77f25a0) to head (0ac82e1).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6726   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files         122      122           
  Lines       37038    37054   +16     
  Branches     2041     2041           
=======================================
+ Hits        36585    36601   +16     
  Misses        314      314           
  Partials      139      139           

Flag	Coverage Δ
CI-GHA	98.66% <100.00%> (+<0.01%)	⬆️
OS-Linux	98.35% <100.00%> (+<0.01%)	⬆️
OS-Windows	96.26% <100.00%> (-0.01%)	⬇️
OS-macOS	97.47% <100.00%> (+<0.01%)	⬆️
Py-3.10.11	97.35% <100.00%> (-0.01%)	⬇️
Py-3.10.15	97.23% <100.00%> (-0.70%)	⬇️
Py-3.10.16	97.73% <100.00%> (?)
Py-3.11.11	98.01% <100.00%> (+<0.01%)	⬆️
Py-3.11.9	97.43% <100.00%> (+<0.01%)	⬆️
Py-3.12.8	98.44% <100.00%> (+<0.01%)	⬆️
Py-3.13.1	98.43% <100.00%> (+<0.01%)	⬆️
Py-3.9.13	97.24% <100.00%> (+<0.01%)	⬆️
Py-3.9.20	97.12% <100.00%> (-0.50%)	⬇️
Py-3.9.21	97.62% <100.00%> (+0.49%)	⬆️
Py-pypy7.3.16	97.39% <100.00%> (+<0.01%)	⬆️
VM-macos	97.47% <100.00%> (+<0.01%)	⬆️
VM-ubuntu	98.35% <100.00%> (+<0.01%)	⬆️
VM-windows	96.26% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codspeed-hq]

CodSpeed Performance Report

Merging #6726 will not alter performance

_{Comparing mdellweg:fix_sub_chunk_file_resonse (e9e0f61) with master (237d467)}

Summary

✅ 47 untouched benchmarks

[mdellweg]

I added a test.
But the weird thing is, I could not make the test fail. Even though I think I found another place where this content length seems to be limited (weirdly so, after compression...) [0]. I even broke that (by commenting out line 126 [0]) and tracked the (now too large) chunk with a debugger to line 82 where it still wastoo large. But the test did not fail. Is it possible that the client used for testing here is more forgiving than it should be (for the purpose of testing anyway)?

How should I proceed?

[0] https://github.com/aio-libs/aiohttp/blob/master/aiohttp/http_writer.py#L126
[1] https://github.com/aio-libs/aiohttp/blob/master/aiohttp/http_writer.py#L82

[Dreamsorcerer]

But the weird thing is, I could not make the test fail.

Check the headers. Seems like Content-Length may be set here:

aiohttp/aiohttp/web_fileresponse.py

Line 396 in 237d467

self.content_length = count

So, if that contains the correct length for the range, then the client should only read that amount of data.

If there's actually more data sent than the header says, then reading the next response should produce an error. So, you could adapt the test to try sending and receiving 2 range requests in a row.

[Dreamsorcerer]

It may also be important to use the context manager correctly if we want to test 2 requests in a row.

Suggested change

	resp = await client.get("/", headers={"Range": "bytes=20-40"})
	assert resp.status == 206
	body = await resp.read()
	async with client.get("/", headers={"Range": "bytes=20-40"}) as resp:
	assert resp.status == 206
	body = await resp.read()

[Dreamsorcerer]

Actually, looking at the code you've edited. Isn't it already doing that in the next couple of lines:

aiohttp/aiohttp/web_fileresponse.py

Line 121 in 237d467

count = count - chunk_size

I think that code cuts it to the correct length. So, your min() is probably not making any difference functionally. It might save reading a little bit of data when it doesn't need it, but wastes a function call when it does, so I'm not clear if it would be more efficient or not.

[mdellweg]

Actually, looking at the code you've edited. Isn't it already doing that in the next couple of lines:

That is the line right after reading and handing down the too big chunk.
I think I don't quite understand what you intend with the next response.
Also i could no longer reproduce the bug in the environment i saw it originally.
As you say, now it's "just" the extra cost of the min call against reading a few more bytes than needed from the IO object.
I'd be fine with just closing this, given that i believe no extra bytes are actually transfered over the wire.

[Dreamsorcerer]

That is the line right after reading and handing down the too big chunk.

Ah, yes, it writes the existing chunk first. So, actually still seems like your fix might be relevant. Did you try the test with 2 range requests in a row?

[mdellweg]

I'll have a shot.

[mdellweg]

Not sure if that's what you meant (looking at the test).
I also broke all the other places that I think prevent sending too many bytes, but still the test would not capture that. The client seems to comply to the content length more than to the tcp stream.

[Dreamsorcerer]

The client seems to comply to the content length more than to the tcp stream.

I think the client should definitely be hitting an error there if there's actually more data sent. Have you tried curl or any other clients and seen it sending more data?

[mdellweg]

No, I haven't. But also by this time, I think aiohttp is doing the right thing (see where i broke the writer too). I would have been happy to contribute at least the test to show that it is. But if it can't get it red it's not testing anything.

[bdraco]

All Python versions we support implement sendfile fallback internally now. So you would likely have to set the env var to reach the fallback code

[bdraco]

Also we have been discussing dropping the aiohttp implementation now that asyncio does it for us

#10096