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.

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

Force connection close after protocol upgrade #3126

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tilgovi
Copy link
Collaborator

@tilgovi tilgovi commented Dec 28, 2023

A new protocol, such as WebSockets or HTTP/2, may manage the framing of multiple messages or requests of that protocol over a single connection, but the connection should close once the WSGI callable returns.

@tilgovi tilgovi force-pushed the fix/upgrade-forces-close branch 2 times, most recently from 8dcc0fd to 1ca598d Compare December 28, 2023 02:27
@tilgovi
Copy link
Collaborator Author

tilgovi commented Dec 28, 2023

RFC 6455 states that closing a WebSocket must close the underlying TCP connection:

After both sending and receiving a Close message, an endpoint considers the WebSocket connection closed and MUST close the underlying TCP connection.

-- https://datatracker.ietf.org/doc/html/rfc6455#section-5.5.1

I couldn't find any language about this in the HTTP/2 protocol, but it seems safer to me to assume that the socket is in an unknown state and any connection persistence is managed at the level of the new protocol.

A new protocol, such as WebSockets or HTTP/2, may manage the framing of
multiple messages or requests of that protocol over a single connection,
but the connection should close once the WSGI callable returns.

- Close connections after completion of any upgraded response.

- Close connections after a response with status code less than 200. Any
  such response should be only for a protocol upgrade since applications
  never see any `Expect` header.

- Ensure that the `status_code` member always exists on instances of the
  `Response` class (close #1210).
if self.upgrade:
# close after the new protocol terminates
return True
if self.response_length is None and not self.chunked:
Copy link
Owner

Choose a reason for hiding this comment

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

why changing the logic there ? Also this doesn't take care anymore about status code < 100. Can you elaborate? (As a side note I'm not a fan of these one line boolean combination, decoupling in multiple if helps to trace the code .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Response' object has no attribute 'status_code' in wsgi.py with websockets
2 participants