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

Client are disconnected only on shutdown #118

Merged
merged 1 commit into from Aug 8, 2014

Conversation

@tuxcanfly
Copy link
Contributor

@tuxcanfly tuxcanfly commented Aug 1, 2014

btcwallet is not letting go of disconnected clients until it's shutdown:

13:53:01 2014-08-01 [INF] BTCW: New websocket client 127.0.0.1:47448
13:53:01 2014-08-01 [INF] BTCW: New websocket client 127.0.0.1:47449
13:53:10 2014-08-01 [WRN] BTCW: Reached threshold of 25 concurrent active clients

^C13:53:23 2014-08-01 [INF] BTCW: Received SIGINT (Ctrl+C).  Shutting down...
13:53:23 2014-08-01 [WRN] BTCW: Server shutting down
13:53:23 2014-08-01 [INF] BTCW: Disconnected websocket client 127.0.0.1:47431
13:53:23 2014-08-01 [INF] BTCW: Disconnected websocket client 127.0.0.1:47426
@tuxcanfly
Copy link
Contributor Author

@tuxcanfly tuxcanfly commented Aug 1, 2014

Closing the responses channel seems to fix this.

Loading

@@ -714,6 +714,7 @@ out:
case request, ok := <-wsc.allRequests:
if !ok {
// client disconnected
close(wsc.responses)
Copy link
Member

@jrick jrick Aug 1, 2014

Choose a reason for hiding this comment

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

This must close responses only after all running handlers for this connection have finished. Since there may be requests still being handled in their own goroutines (see the go func in the default case), this will panic if a send occurs after the channel is already closed.

Unfortunately, the waitgroup used to count the started goroutines that are still running (and that send to responses) is the RPC server waitgroup, and is not specific to this websocket client. I think the best solution here would be to introduce a new waitgroup variable in this function, and close over it in each handler goroutines. Then, right before the s.wg.Done() at the bottom, this should handlerWg.Wait() and then close(wsc.responses).

Loading

@tuxcanfly
Copy link
Contributor Author

@tuxcanfly tuxcanfly commented Aug 6, 2014

@jrick I've updated to use a different waitgroup specific to the client as you suggested. What do you think about having this waitgroup on websocketClient itself, like in the last commit?

Loading

added a waitgroup on websocketClient to keep track of handler
goroutines specific to the client
@conformal-deploy conformal-deploy merged commit 8759d12 into btcsuite:master Aug 8, 2014
1 check passed
Loading
@tuxcanfly
Copy link
Contributor Author

@tuxcanfly tuxcanfly commented Aug 8, 2014

Thanks!

Loading

jrick added a commit to jrick/btcwallet that referenced this issue Nov 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants