-
Notifications
You must be signed in to change notification settings - Fork 583
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
base: master
Are you sure you want to change the base?
Conversation
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.
It looks like the tests are failing. I tried re-running, but it may be an actual failure. I haven't looked into why. |
Ah well, missed the case where the server closes the connection. |
@kim looks like it's still failing? |
meh |
There was a problem hiding this 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
There was a problem hiding this 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. 🚢
The
subscribe
command would drop the connection without sending aclose 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 beforeand after.