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

Bump websockets to 11.0.3. #2061

Merged
merged 1 commit into from
Jul 30, 2023
Merged

Conversation

aaugustin
Copy link
Contributor

Fix #1927.

@aaugustin aaugustin mentioned this pull request Jul 29, 2023
@aaugustin aaugustin force-pushed the bump-websockets-11 branch from 0623097 to 22f880d Compare July 29, 2023 07:20
Fix #1927.

Change ws_handler to wait for the transport to be closed before exiting.
This prevents a performance optimization in websockets to cause a test
failure. Also, it aligns the behavior between wsproto and websockets.
@aaugustin aaugustin force-pushed the bump-websockets-11 branch from 22f880d to 24fa3d0 Compare July 30, 2023 12:18
@aaugustin
Copy link
Contributor Author

aaugustin commented Jul 30, 2023

FYI: I tried to remove self.closed_event entirely:

  • await self.closed_event.wait() -> await self.wait_closed()
  • self.closed_event.is_set() -> self.closed

but it caused another test failure. I didn't investigate further.

@aaugustin
Copy link
Contributor Author

This change guarantees that the connection handler never exits before the TCP connection is closed. Effectively, this disables websockets' automatic closing handshake, which is clearly the intent of the current implementation (but not always true).

@Kludex
Copy link
Member

Kludex commented Jul 30, 2023

For the record (for myself), I've tested locally with the application in the test suite:

async def app(scope, receive, send):
    await send({"type": "websocket.accept"})
    return 123

Using a different WebSockets client (wscat):

❯ wscat -c "ws://localhost:8000/"

And I got the same 1006 as before.

Copy link
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.

Thanks @aaugustin 🙏

@Kludex Kludex merged commit c6eb011 into encode:master Jul 30, 2023
@aaugustin aaugustin deleted the bump-websockets-11 branch July 30, 2023 17:16
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.

WebSockets 11.0 broke Uvicorn test suite
2 participants