Make UploadFile check for future rollover #2962
Conversation
Signed-off-by: Michael Honaker <mchonaker@gmail.com>
Signed-off-by: Michael Honaker <mchonaker@gmail.com>
Signed-off-by: Michael Honaker <mchonaker@gmail.com>
Signed-off-by: Michael Honaker <mchonaker@gmail.com>
Signed-off-by: Michael Honaker <mchonaker@gmail.com>
tests/test_formparsers.py
Outdated
| def test_multipart_request_large_file(tmpdir: Path, test_client_factory: TestClientFactory) -> None: | ||
| data = BytesIO(b" " * MultiPartParser.spool_max_size * 2) | ||
| client = test_client_factory(app) | ||
| response = client.post( | ||
| "/", | ||
| files=[("test_large", data)], | ||
| ) | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
This test passes on master. We want some condition that proves this PR actually solves the issue.
There was a problem hiding this comment.
This type of issue is hard to verify without profiling or adding breakpoints and checking if a write operation that caused a rollover happened on the main thread. I do not know if it is feasable or even possible to provide a unit test that fails if the blocking rollover happens on the wrong thread, that would require assert statements and additional logic in the actual code.
There was a problem hiding this comment.
Then we can remove the test, can't we?
There was a problem hiding this comment.
@Kludex yeah in it's current state it's not doing a whole lot. Let met try editing the test to use a monkey patched SpooledTemporaryFile to ensure the IO thread is separate from the event thread
There was a problem hiding this comment.
It does not ensure that there is no blocking call on the event loop, but it does cover the new code paths that would otherwise not be covered.
There was a problem hiding this comment.
@Kludex Just pushed the new test! I ended up creating a new class/app to track where rollover was happening. I'm still new to starlette development so please let me know if you have any code-quality suggestions.
Signed-off-by: Michael Honaker <mchonaker@gmail.com>
Signed-off-by: Michael Honaker <mchonaker@gmail.com>
HonakerM
force-pushed
the
fix_upload_file_hang
branch
from
4a27556 to
297c09e
Compare
|
Not for this PR, but at this point I would just move the rollover logic into |
Summary
This PR ensures that during multi-part form parsing any slow disk IO happens in a background. This specifically fixes a case where a file would rollover during a
write()call as talked about in this discussion.Checklist