Skip to content

Conversation

rekhoff
Copy link
Contributor

@rekhoff rekhoff commented Oct 3, 2025

Description of Changes

The implementation of a solution to #3044 , this adds an Abort function to the WebSocket, which runs if Disconnect is called when the WebSocket is not connected.

API and ABI breaking changes

Not API breaking.

Expected complexity level and risk

1

Testing

  • Test locally with a C# CLI test client.
    Note: Before change (either on Rust of C# server), server would see 4 Debug log entries about connecting, but not see the Info log about the client connection ending like would normally be seen in a disconnect. After change, server shows no log entries at all, because connection is properly aborted.
  • Test locally with a C# WebGL test client.

@rekhoff rekhoff self-assigned this Oct 3, 2025
@rekhoff rekhoff requested a review from jdetter October 3, 2025 15:49

public void Disconnect()
{
isClosing = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is somewhat unrelated but we might want to clean up this logic as well. If the socket isn't in the connected or connecting states we should just not try to disconnect. If the socket is connected we should try a normal disconnection and if the socket is connecting then we should try to abort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More or less that is what this is doing. If DBConnection.Disconnect is called by the client, the Disconnect function is checking if the websocket is connected, and if it isn't actually connected yet, it's calling Abort.
But to your point, I've added a check to ensure we only call Abort if we are in the process of connecting, so we don't accidently call it if we are already disconnecting or are in a closed status.

@rekhoff rekhoff marked this pull request as ready for review October 3, 2025 17:31
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