[RESEND] Fix HttpPayloadParser dealing with chunked response (#4630) #4846
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If the last CRLF or only the LF are received via separate TCP segment, HTTPPayloadParser misjudges that trailers should come after 0\r\n in the chunked response body. In this case, HttpPayloadParser starts waiting for trailers, but the only remaining data to be received is CRLF. Thus, HttpPayloadParser waits trailers indefinitely and this incurs TimeoutError in user code. However, if the connection is keep alive disabled, this problem is not reproduced because the server shutdown the connection explicitly after sending all data. If the connection is closed .feed_eof is called and it helps HttpPayloadParser finish its waiting.
PR aio-libs#4651 by @JustAnotherArchivist This change adds tests that demonstrate the failures described in aio-libs#4630. They are marked as xfail so that they don't affect the CI status. Once the issue is fixed, they'll reported as XPASS and pytest will fail, which would be a signal that it's time to remove the xfail markers keeping the contents of the tests to prevent regressions. (ref: https://pganssle-talks.github.io/xfail-lightning) Co-Authored-By: Sviatoslav Sydorenko <[email protected]>
psf-chronographer
bot
added
the
bot:chronographer:provided
Codecov Report
@@ Coverage Diff @@
## 3.7 #4846 +/- ##
=======================================
Coverage 97.89% 97.90%
=======================================
Files 44 44
Lines 8951 8957 +6
Branches 1407 1409 +2
=======================================
+ Hits 8763 8769 +6
Misses 80 80
Partials 108 108
Continue to review full report at Codecov.
|
|
The errors of tests are related to aio-libs/yarl#410 |
4 tasks
|
Hi @socketpair |
|
Unfortunately tests fail on Python 3.8. Anyway I'm sure this was not triggerd by the PR. So, I would merge @asvetlov. |
asvetlov
added a commit
that referenced
this pull request
Oct 16, 2020
* Parse the last CRLF of chunked response correctly (#4630) If the last CRLF or only the LF are received via separate TCP segment, HTTPPayloadParser misjudges that trailers should come after 0\r\n in the chunked response body. In this case, HttpPayloadParser starts waiting for trailers, but the only remaining data to be received is CRLF. Thus, HttpPayloadParser waits trailers indefinitely and this incurs TimeoutError in user code. However, if the connection is keep alive disabled, this problem is not reproduced because the server shutdown the connection explicitly after sending all data. If the connection is closed .feed_eof is called and it helps HttpPayloadParser finish its waiting. Co-authored-by: JustAnotherArchivist <[email protected]> Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>
Closed
asvetlov
added a commit
that referenced
this pull request
Oct 16, 2020
* Parse the last CRLF of chunked response correctly (#4630) If the last CRLF or only the LF are received via separate TCP segment, HTTPPayloadParser misjudges that trailers should come after 0\r\n in the chunked response body. In this case, HttpPayloadParser starts waiting for trailers, but the only remaining data to be received is CRLF. Thus, HttpPayloadParser waits trailers indefinitely and this incurs TimeoutError in user code. However, if the connection is keep alive disabled, this problem is not reproduced because the server shutdown the connection explicitly after sending all data. If the connection is closed .feed_eof is called and it helps HttpPayloadParser finish its waiting. Co-authored-by: JustAnotherArchivist <[email protected]> Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello, @socketpair
I rebased my commits onto origin/3.7 and resend them.
I cherry-picked @JustAnotherArchivist 's commit because it belongs to the master branch but not to the origin/3.7 branch.
The previous PR which was based on origin/master: #4801
HttpPayloadParser waits for trailers indefinitely even if there are no trailers
at the response. This happens when only the last CRLF or the last LF are sent
via separate TCP segment.
When the connection is keep alive and if this bug occurs then users experience
response timeout. But this problem is not exposed when keep alive is disabled
because .feed_eof is called. (Instead of TimeoutError, ClientPayloadError is raised if keep alive is disabled)
What do these changes do?
Fix a bug that HttpPayloadParser waits for data indefintely that will never come.
The bug makes caller of 'await response.read()' awaits forever or for timeout.
There are a few conditions which need to be met in order to reproduce this bug.
Are there changes in behavior for the user?
Improvement experience of users who are suffering from mysterous response timeout. There does not exist any log of which response time is slow in sever access log, but client writes log about response timeout.
Related issue number
#4630
I had a problem with intermittent response timeout so I conducted debug a lot.
And I managed to locate what caused the problem and fixed it on my own.
And then I was going to add test code that prove my modification really fix that thing. So I learned how to run test code in this project. And I ran test codes for the first time.
Surprisingly the issue 4630 just popped up with XPASS labeled. At this point I realized that the issue had been already reported 3 months ago. Sigh.. If the issue was fixed at that time, I would not spend my time for debugging this problem!
I did not mean to provide the fix on behalf of the person who reported the issue before me. I just did not know there was the issue before I started fixing the bug.
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.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.