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.

Sign up for GitHub

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

Jump to bottom

Fix 0.30.4 issue with connection close header #2408

Merged
merged 4 commits into from
Aug 2, 2024
Merged

Fix 0.30.4 issue with connection close header #2408

merged 4 commits into from
Aug 2, 2024

Conversation

theyashl
Copy link
Contributor

@theyashl theyashl commented Aug 1, 2024

Summary

When connection: close header is passed to uvicorn server, RequestResponseCycle's receive method goes into waiting state. Resulting in request timeout or explicit connection close action from the client.
This PR will fix the issue.
Detailed RCA: #2375 (comment)

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

Kludex
Kludex requested changes Aug 2, 2024
View reviewed changes
Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

The previous test was good, no need to modify it.

Your test is not representative, is it? Can we have a test with a POST request and connection: close header?

@theyashl
Copy link
Contributor Author

theyashl commented Aug 2, 2024

Hi @Kludex ,
Previous test case was relying on creating an app using test Response class. Which did not had await receive() statement.

While pre-existing test_post_request test case also have implemented app definition explicitly.

@theyashl theyashl mentioned this pull request Aug 2, 2024
Merged
3 tasks
@Kludex
Copy link
Sponsor Member

Kludex commented Aug 2, 2024

While pre-existing test_post_request test case also have implemented app definition explicitly.

Yeah, but it's a POST request, and it's reading the body. But that test doesn't have the connection: close.

Previous test case was relying on creating an app using test Response class. Which did not had await receive() statement.

It's a valid test anyway.

@theyashl
Copy link
Contributor Author

theyashl commented Aug 2, 2024

Ahh got your point. So I can just implement a POST test case with Connection: close header.

Should I include that test case too? While reverting the current one back to the original state.

@Kludex
Copy link
Sponsor Member

Kludex commented Aug 2, 2024

Ahh got your point. So I can just implement a POST test case with Connection: close header.

I mean, that's the case you want to really handle here, right? If yes, then the answer is yes.

Should I include that test case too? While reverting the current one back to the original state.

Yes, please.

Kludex
Kludex requested changes Aug 2, 2024
View reviewed changes
tests/protocols/test_http.py Outdated
]
)

CONNECTION_CLOSE_POST_REQUEST_WITHOUT_BODY = b"\r\n".join(
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

You can remove this one. The above case is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove this case of POST without body. Kept POST with body and connection close header

tests/protocols/test_http.py Outdated
@@ -998,8 +1014,8 @@ async def test_return_close_header(http_protocol_cls: HTTPProtocol):


async def test_close_connection_with_multiple_requests(http_protocol_cls: HTTPProtocol):
app = Response("Hello, world", media_type="text/plain")

response_content = b"Hello, world"
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Don't add a diff here please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed diffs

@Kludex Kludex enabled auto-merge (squash) August 2, 2024 08:11
@Kludex Kludex merged commit 2f25107 into encode:master Aug 2, 2024
15 checks passed
@Konano
Copy link

Konano commented Aug 2, 2024 •
edited
Loading

I was wondering why the newly installed uvicorn wasn't responding. Turns out, the problem was here!

Although I managed to pinpoint the issue myself, it looks like it's already been fixed.

This bug has a significant impact, so I hope a new version can be released soon. Thanks!

@Kludex
Copy link
Sponsor Member

Kludex commented Aug 2, 2024

This is fixed in 0.30.5.

@theyashl
Copy link
Contributor Author

theyashl commented Aug 2, 2024

Sorry for the problem caused due to the previous fix. :)

@Kludex
Copy link
Sponsor Member

Kludex commented Aug 2, 2024

Sorry for the problem caused due to the previous fix. :)

Nah, it's fine. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kludex Kludex Kludex approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@theyashl @Kludex @Konano