Skip to content

Conversation

@HaraldNordgren
Copy link
Contributor

@HaraldNordgren HaraldNordgren commented Oct 26, 2025

Fixes #399.

@HaraldNordgren HaraldNordgren changed the title websocket: do unsubscribe before closing connection websocket: unsubscribe before closing connection Oct 26, 2025
@HaraldNordgren HaraldNordgren marked this pull request as draft October 26, 2025 19:44
@HaraldNordgren HaraldNordgren force-pushed the unsub_before_closing branch 3 times, most recently from 949ecf6 to 9a5c3d9 Compare October 26, 2025 22:51
@HaraldNordgren HaraldNordgren changed the title websocket: unsubscribe before closing connection websocket: gracefully close connection Oct 27, 2025
@HaraldNordgren HaraldNordgren marked this pull request as ready for review October 27, 2025 08:30
@HaraldNordgren
Copy link
Contributor Author

Ping @benjasper @benjaminjkraft

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

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

Thanks!

@benjaminjkraft benjaminjkraft merged commit ab60004 into Khan:main Oct 28, 2025
5 checks passed
@benjaminjkraft
Copy link
Collaborator

benjaminjkraft commented Oct 28, 2025

@HaraldNordgren it looks like there's a test flake possibly due to this?

benjaminjkraft added a commit that referenced this pull request Oct 28, 2025
The people want a release! (See discussion on #394, #383, fixes #396.)

I'll wait to merge this until the discussion on #402 is resolved, since the test flake looks like a deadlock, and I can't tell offhand if it could affect real usage.
@benjaminjkraft benjaminjkraft mentioned this pull request Oct 28, 2025
@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Oct 28, 2025

@benjaminjkraft Ouch, this is bad. If I just add this

diff --git a/internal/integration/integration_test.go b/internal/integration/integration_test.go
index 88e9238..99b6055 100644
--- a/internal/integration/integration_test.go
+++ b/internal/integration/integration_test.go
@@ -281,6 +281,7 @@ func TestSubscriptionClose(t *testing.T) {
 			require.NoError(t, err)
 
 			if tc.unsub {
+				time.Sleep(100 * time.Millisecond)
 				err = wsClient.Unsubscribe(subscriptionID)
 				require.NoError(t, err)
 			}

then it gets stuck every time.

@HaraldNordgren
Copy link
Contributor Author

HaraldNordgren commented Oct 28, 2025

@benjaminjkraft Draft for possible fix: #407

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.

Close subscription client gracefully

2 participants