Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sending zero byte files after sendfile changes #5380

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Jan 2, 2021

What do these changes do?

Changing the sendfile implementation in #5157 caused a
regression with sending zero byte files, and the test
had too much mocking to expose the issue.

Are there changes in behavior for the user?

Bugfix only

Related issue number

#5157
#5124
home-assistant/core#42514

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."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 2, 2021
@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #5380 (d1763e9) into master (3edc43c) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5380      +/-   ##
==========================================
- Coverage   97.16%   97.15%   -0.02%     
==========================================
  Files          41       41              
  Lines        8746     8746              
  Branches     1402     1402              
==========================================
- Hits         8498     8497       -1     
  Misses        129      129              
- Partials      119      120       +1     
Flag Coverage Δ
unit 97.03% <100.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
aiohttp/web_fileresponse.py 99.18% <100.00%> (-0.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3edc43c...d1763e9. Read the comment docs.

@bdraco bdraco marked this pull request as ready for review January 2, 2021 20:35
@bdraco bdraco requested a review from asvetlov as a code owner January 2, 2021 20:35
Changing the sendfile implementation in aio-libs#5157 caused a
regression with sending zero byte files, and the test
had too much mocking to expose the issue.
@joshuaspence
Copy link

joshuaspence commented Feb 8, 2021

I'm thinking this will fix the following error that I am seeing a few times per day?

2021-02-08 13:26:25 ERROR (MainThread) [aiohttp.server] Unhandled exception
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/aiohttp/web_protocol.py", line 485, in start
    resp, reset = await task
  File "/usr/local/lib/python3.8/site-packages/aiohttp/web_protocol.py", line 440, in _handle_request
    reset = await self.finish_response(request, resp, start_time)
  File "/usr/local/lib/python3.8/site-packages/aiohttp/web_protocol.py", line 591, in finish_response
    await prepare_meth(request)
  File "/usr/local/lib/python3.8/site-packages/aiohttp/web_fileresponse.py", line 241, in prepare
    return await self._sendfile(request, fobj, offset, count)
  File "/usr/local/lib/python3.8/site-packages/aiohttp/web_fileresponse.py", line 96, in _sendfile
    await loop.sendfile(transport, fobj, offset, count)
  File "/usr/local/lib/python3.8/asyncio/base_events.py", line 1120, in sendfile
    return await self._sendfile_native(transport, file,
  File "/usr/local/lib/python3.8/asyncio/selector_events.py", line 578, in _sendfile_native
    return await self.sock_sendfile(transp._sock, file, offset, count,
  File "/usr/local/lib/python3.8/asyncio/base_events.py", line 836, in sock_sendfile
    self._check_sendfile_params(sock, file, offset, count)
  File "/usr/local/lib/python3.8/asyncio/base_events.py", line 889, in _check_sendfile_params
    raise ValueError(
ValueError: count must be a positive integer (got 0)

@bdraco
Copy link
Member Author

bdraco commented Feb 8, 2021

ValueError: count must be a positive integer (got 0)

Yes it should

@djrm05
Copy link

djrm05 commented Feb 26, 2021

ValueError: count must be a positive integer (got 0)

Yes it should

I was searching for the same issue and got this page. Could you pls let me know if this will be merge to the live version? I'm running HA 2021.2.3 in docker and seeing the above error in logs... Thanks!

@gralance
Copy link

gralance commented Jul 8, 2021

Hello @asvetlov
will this fix be merged in the next release?

@cdonovan
Copy link

Hey @asvetlov, is there any chance this could get reviewed for release soon? I verified @bdraco's fix via Home Assistant (manually patched web_fileresponse.py in my docker container).

@webknjaz webknjaz added this to the 3.8 milestone Oct 27, 2021
Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@asvetlov asvetlov merged commit 11b46df into aio-libs:master Oct 27, 2021
@patchback
Copy link
Contributor

patchback bot commented Oct 27, 2021

Backport to 3.8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.8/11b46df4215d4802a0de9aa4f5b3d25a874431d0/pr-5380

Backported as #6145

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Oct 27, 2021
Changing the sendfile implementation in #5157 caused a
regression with sending zero byte files, and the test
had too much mocking to expose the issue.

(cherry picked from commit 11b46df)
asvetlov pushed a commit that referenced this pull request Oct 27, 2021
Changing the sendfile implementation in #5157 caused a
regression with sending zero byte files, and the test
had too much mocking to expose the issue.

(cherry picked from commit 11b46df)

Co-authored-by: J. Nick Koston <nick@koston.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants