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

Send disconnect event on connection lost for wsproto #996

Merged
merged 5 commits into from Oct 29, 2022

Conversation

sephioh
Copy link
Contributor

@sephioh sephioh commented Mar 9, 2021

Using uvicorn with wsproto implementation, if clients do not close websocket connections properly, no exc will be sent to connection_lost (see code below).

As in the current version of uvicorn, this creates two issues:

  • apps will not receive websocket.disconnect events;
  • a zombie run_asgi (asyncio) task running for each non properly closed connection;

In order to reproduce the issue, follow instructions on this repo: https://github.com/sephioh/uvicorn-wsproto-issue

Edit by @Kludex :

@euri10
Copy link
Member

euri10 commented May 20, 2021

seems like the exception check was added in 3cab132#diff-5f163724db1eaba811cc5e4ed55e529af488e351ee52ccfb7c1ad0329a8f0d92R65-R66 and I dont get the rationale given the context, I know it's been a while but do you remember why @almarklein or @tomchristie ?

@euri10
Copy link
Member

euri10 commented May 20, 2021

Fixes #997

@Kludex
Copy link
Sponsor Member

Kludex commented Oct 28, 2022

Thank you for the PR @sephioh !

Also, sorry for the long waiting time.

Kludex
Kludex approved these changes Oct 28, 2022
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.

I'll add a test for this before merging it.

@Kludex Kludex added this to the Version 0.20.0 milestone Oct 28, 2022
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@Kludex Kludex changed the title queue websocket disconnect for dropped connections Send websocket.disconnect event when client lost connection on wsproto Oct 29, 2022
@Kludex Kludex changed the title Send websocket.disconnect event when client lost connection on wsproto Send websocket.disconnect event when client loses connection on wsproto Oct 29, 2022
@Kludex Kludex changed the title Send websocket.disconnect event when client loses connection on wsproto Send disconnect event on connection lost for wsproto Oct 29, 2022
@Kludex Kludex merged commit 3243f20 into encode:master Oct 29, 2022
15 checks passed
Kludex added a commit that referenced this pull request Oct 29, 2022
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants