FileResponse has undefined behavior if the file is changed out from under it between the stat and open calls
#8013
Comments
We didn't have any benchmarks for file responses. From the benchmarks it turns out most of the time is creating and processing the executor jobs. If we combine the stat into a job that returns the open fileobj it will likely be faster and solve #8013
|
It looks like this is worth fixing as it will also give a nice performance boost by reducing one executor job per request #10095 |
👍🏻 but I'd add a benchmark for a 304 response as well. Those would end up with a penalty from any overhead opening the file and the extra executor job to close it (4xx responses would also be penalized but less a concern I guess). |
…nse (#10098) **This is a backport of PR #10095 as merged into master (fcce1bf).** We didn't have any benchmarks for file responses. From the benchmarks it turns out most of the time is creating and processing the executor jobs. If we combine the stat into a job that returns the open fileobj it will likely be faster and solve #8013 Co-authored-by: J. Nick Koston <[email protected]>
…nse (#10097) **This is a backport of PR #10095 as merged into master (fcce1bf).** We didn't have any benchmarks for file responses. From the benchmarks it turns out most of the time is creating and processing the executor jobs. If we combine the stat into a job that returns the open fileobj it will likely be faster and solve #8013 Co-authored-by: J. Nick Koston <[email protected]>
…d open calls There was a race in ``FileResponse`` where the stat would be incorrect if the file was changed out between the `stat` and `open` syscalls. This would lead to various unexpected behaviors such as trying to read beyond the length of the file or sending a partial file. This problem is likely to occour when files are being renamed/linked into place. An example of how this can happen with a system that provides weather data every 60s: An external process writes `.weather.txt` at the top of each minute, and than renames it to `weather.txt`. In this case `aiohttp` may stat the old `weather.txt`, and than open the new `weather.txt`, and use the `stat` result from the original file. To fix this we now `fstat` the open file on operating systems where `fstat` is available fixes #8013
I kept that in mind when I wrote the fix. The number of executor jobs remains the same for both a success and 304 response (not considering the close one which is no change). If you would like to add a benchmark for 304s as well in a fresh PR, that would be a nice addition to ensure we don't regress it in the future. |
…e is replaced during `prepare` (#10105) Co-authored-by: J. Nick Koston <[email protected]> fixes #8013
…e is replaced during `prepare` (#10106) Co-authored-by: J. Nick Koston <[email protected]> fixes #8013
|
The fix certainly handles the closings smartly, but I would be more worried about the additional overhead of the file openings. If cache is controlled well, many implementations are likely serving 304s much more than 200s. #10101 probably introduced a minor penalty there, but I don't see a better way to fix this issue. 🤷🏻♂️ |
@steverep |
This was discovered while fixing another issue in #8012 (comment)
This is a rare case so its not likely we need to prioritize fixing it but I'm sure this race has bitten someone with random unexplained failures who uses aiohttp to serve index or signature files that get changed out frequently.
Describe the bug
We can fix this by
opening the file first, doingfstatto get thestatTo Reproduce
replace the file on the FS between the
statandopencallExpected behavior
The handler should still send the original file
Logs/tracebacks
Python Version
$ python --versionaiohttp Version
$ python -m pip show aiohttpmultidict Version
$ python -m pip show multidictyarl Version
$ python -m pip show yarlOS
n/a
Related component
Server
Additional context
n/a
Code of Conduct
The text was updated successfully, but these errors were encountered: