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

fix some issues with ClientDisconnect #5625

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MilonPL
Copy link
Contributor

@MilonPL MilonPL commented Jan 21, 2025

fixes an issue with ClientDisconnect (and by extension the DisconnectCommand)

turns out that DisconnectCommand is sort of a fucky-wucky

  • If you use it while attempting to connect to a server, you get stuck in quantum superposition state of not connecting while connecting

image

  • if you use it while attempting to connect in dev env, you get stuck in a void

image

the issue was that we weren't properly handling disconnects during active connection attempts - the code assumed you were already connected when trying to disconnect (2017 code my beloved)

now you can actually disconnect while connecting without breaking the universe 👍

@MilonPL
Copy link
Contributor Author

MilonPL commented Jan 22, 2025

@PJB3005 ready for re-review
Fixed it by handling cancellation in CCHappyEyeballs, turns out we weren't catching cancelled connection attempts during the handshake when connecting to non-responsive servers

@MilonPL MilonPL requested a review from PJB3005 January 22, 2025 14:45
Copy link
Member

@PJB3005 PJB3005 left a comment

Choose a reason for hiding this comment

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

Adding that try catch to AwaitNonInitStatusChange doesn't seem like it makes a lot of sense to me.

Reading the code, there's already a try catch for cancellation around most of the logic, but it seems like it's erroneously catching on TaskCanceledException instead of OperationCanceledException (definitely my fault). Does fixing that fix it?

@PJB3005
Copy link
Member

PJB3005 commented Jan 24, 2025

Also figured I'd mention this: we can probably replace most of the logic in CCHappyEyeballs with usage of the HappyEyeballsHttp.ParallelTask<T> that I've since created. But it's really not that important so whatever.

@MilonPL MilonPL requested a review from PJB3005 January 31, 2025 16:25
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.

2 participants