Skip to content

cli: Close the websocket connection gracefully #2925

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

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

Conversation

kim
Copy link
Contributor

@kim kim commented Jul 7, 2025

The subscribe command would drop the connection without sending a
close frame. Doing so creates a warning on the server, which can be
distracting when debugging other connections issues.

Expected complexity level and risk

1

Testing

Use the subscribe command with a local database and compare server logs before
and after.

The `subscribe` command would drop the connection without sending a
close frame. Doing so creates a warning on the server, which can be
distracting when debugging other connections issues.
@bfops
Copy link
Collaborator

bfops commented Jul 7, 2025

It looks like the tests are failing. I tried re-running, but it may be an actual failure. I haven't looked into why.

@bfops bfops added release-any To be landed in any release window bugfix Fixes something that was expected to work differently labels Jul 7, 2025
@kim
Copy link
Contributor Author

kim commented Jul 8, 2025

Ah well, missed the case where the server closes the connection.

@kim kim requested a review from bfops July 8, 2025 08:51
@bfops
Copy link
Collaborator

bfops commented Jul 8, 2025

@kim looks like it's still failing?

@kim
Copy link
Contributor Author

kim commented Jul 8, 2025

meh

Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

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

I haven't carefully reviewed the new Error type variants, but the rest of the code that constructs them seems fine. It's fine if the error messages changed a bit or something.

A couple of small questions

Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

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

This PR LGTM. I haven't done any testing.

There's still one unresolved comment thread, that you can resolve however makes sense to you and then merge. 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes something that was expected to work differently release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants