Skip to content

Fix FileResponse doing blocking I/O in the event loop#8012

Merged
bdraco merged 2 commits intomasterfrom
file_response_io_event_loop
Jan 10, 2024
Merged

Fix FileResponse doing blocking I/O in the event loop#8012
bdraco merged 2 commits intomasterfrom
file_response_io_event_loop

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented Jan 9, 2024 •
edited
Loading

What do these changes do?

Fix FileResponse doing blocking I/O in the event loop

The call to os.stat was run in the executor but the call to is_file() was not. Additionally this resulted in two syscalls to stat since both is_file and stat will call stat under the hood. The code has been refactored to ensure there is only one stat syscall and it happens in the executor

Are there changes in behavior for the user?

No

Related issue number

discovered while investigating #8011 but not does fix the issue reported

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

@bdraco
Fix FileResponse doing blocking I/O in the event loop
bb1609d 
The call to os.stat was run in the executor but the
call to `is_file()` was not. Additionally this resulted
in two syscalls to `stat` since both `is_file` and
`stat` will call `stat` under the hood. The code
has been refactored to ensure there is only one
`stat` syscall and it happens in the executor
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided There is a change note present in this PR label Jan 9, 2024
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Jan 9, 2024 •
edited
Loading

There is also a race here since nothing guarantees that the file can't change between the time we stat it and the time we open it

Fixing that is a lot harder though since we should open it , fstat it instead and keep it open, and than we have to deal with making sure its closed.

I opened an issue for that so we don't forget about it, but its such a rare case that I don't think we need to prioritize fixing it
#8013

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6529c77) 97.46% compared to head (0edc20f) 97.46%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #8012   +/-   ##
=======================================
  Coverage   97.46%   97.46%           
=======================================
  Files         107      107           
  Lines       32526    32529    +3     
  Branches     3791     3790    -1     
=======================================
+ Hits        31700    31703    +3     
  Misses        624      624           
  Partials      202      202
Flag Coverage Δ
CI-GHA 97.37% <100.00%> (+<0.01%) ⬆️
OS-Linux 97.05% <100.00%> (+<0.01%) ⬆️
OS-Windows 95.54% <100.00%> (+<0.01%) ⬆️
OS-macOS 96.68% <100.00%> (+<0.01%) ⬆️
Py-3.10.11 95.47% <100.00%> (+<0.01%) ⬆️
Py-3.10.13 96.86% <100.00%> (+<0.01%) ⬆️
Py-3.11.7 96.57% <100.00%> (+<0.01%) ⬆️
Py-3.12.1 96.64% <100.00%> (+<0.01%) ⬆️
Py-3.8.10 95.44% <100.00%> (+<0.01%) ⬆️
Py-3.8.18 96.79% <100.00%> (+<0.01%) ⬆️
Py-3.9.13 95.44% <100.00%> (+<0.01%) ⬆️
Py-3.9.18 96.82% <100.00%> (-0.01%) ⬇️
Py-pypy7.3.13 96.38% <100.00%> (+<0.01%) ⬆️
VM-macos 96.68% <100.00%> (+<0.01%) ⬆️
VM-ubuntu 97.05% <100.00%> (+<0.01%) ⬆️
VM-windows 95.54% <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.

@bdraco bdraco mentioned this pull request Jan 9, 2024
Closed
1 task
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Jan 9, 2024

local testing looks good

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Jan 9, 2024

looks good on a second production system.

doing a py-spy to check the execution path

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Jan 9, 2024

looks good

@bdraco bdraco marked this pull request as ready for review January 9, 2024 22:18
@bdraco bdraco requested a review from asvetlov as a code owner January 9, 2024 22:18
Dreamsorcerer
Dreamsorcerer reviewed Jan 10, 2024
View reviewed changes
Comment thread aiohttp/web_fileresponse.py
@bdraco bdraco merged commit 5f699bb into master Jan 10, 2024
@bdraco bdraco deleted the file_response_io_event_loop branch January 10, 2024 23:39
@patchback
Copy link
Copy Markdown
Contributor

patchback Bot commented Jan 10, 2024 •
edited
Loading

Backport to 3.9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.9/5f699bbbf98d3700921ec9a7d646d5f21e82e8c1/pr-8012

Backported as #8015

🤖 @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 Jan 10, 2024
@bdraco @patchback
Fix FileResponse doing blocking I/O in the event loop (#8012)
abbe2d8 
(cherry picked from commit 5f699bb)
@patchback patchback Bot mentioned this pull request Jan 10, 2024
Merged
5 tasks
@patchback
Copy link
Copy Markdown
Contributor

patchback Bot commented Jan 10, 2024 •
edited
Loading

Backport to 3.10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.10/5f699bbbf98d3700921ec9a7d646d5f21e82e8c1/pr-8012

Backported as #8016

🤖 @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 Jan 10, 2024
@bdraco @patchback
Fix FileResponse doing blocking I/O in the event loop (#8012)
e3408ec 
(cherry picked from commit 5f699bb)
@patchback patchback Bot mentioned this pull request Jan 10, 2024
Merged
5 tasks
bdraco added a commit that referenced this pull request Jan 11, 2024
@patchback @bdraco
[PR #8012/5f699bbb backport][3.10] Fix FileResponse doing blocking I/…
8de4188 
…O in the event loop (#8016)

Co-authored-by: J. Nick Koston <nick@koston.org>
bdraco added a commit that referenced this pull request Jan 11, 2024
@patchback @bdraco
[PR #8012/5f699bbb backport][3.9] Fix FileResponse doing blocking I/O…
fca5a7a 
… in the event loop (#8015)

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

Reviewers

@Dreamsorcerer Dreamsorcerer Dreamsorcerer approved these changes

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees

No one assigned

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bdraco @Dreamsorcerer