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

Stop goroutines when closing a connection #39

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@stolyaroleh

stolyaroleh commented May 18, 2017

Hello, @BurntSushi. I would like to use xgb in one of my projects. It's a service that runs in background and talks to the X server on behalf of all currently logged in users. Since it maintains a pool of X11 connections that can die at any moment (for example, when an interactive session stops on logout), I wanted to make sure that xgb will not panic/leave garbage behind when this happens.

This pull request attempts to fix #32, by introducing a sync.WaitGroup and blocking in Close() until all 4 goroutines, spawned by NewConnNet die. I tested it by creating and closing 100 000 connections - decreased RAM usage from at least 700 Mb to ~25 Mb.

Additionally, I use xgb.Conn.done (previously xgb.Conn.closing, a chan struct{}) to broadcast that all goroutines need to stop (receive on closed channel does not block).

Finally, I broadcastDone on unrecoverable read errors in readResponses instead of xgb.Conn.Close(). Since readResponses contains an infinite loop, and previously would just continue after Close() (line 406, xgb.go), it would attempt to Close() again in the next iteration, causing a panic (closing an already closed channel).

I don't have much experience with xgb or X11 in general and would greatly appreciate if you had a look at this, in case I missed something.

func (c *Conn) broadcastDone() {
select {
case <-c.done:

This comment has been minimized.

@BurntSushi

BurntSushi Aug 17, 2017

Owner

What sends on this channel?

@BurntSushi

BurntSushi Aug 17, 2017

Owner

What sends on this channel?

This comment has been minimized.

@stolyaroleh

stolyaroleh Aug 17, 2017

When readResponses hits an unrecoverable read error on line 435 and 463, it closes this channel by calling broadcastDone, thus signaling to all goroutines that they should shutdown. Since receive on a closed channel does not block, having:

select {
case <-c.done:
   cleanup()
   return
case ...:
   ...
}

in a goroutine will shut it down after broadcastDone.

I am doing a nonblocking select here to prevent c.done from being closed twice, since that will trigger a panic.

@stolyaroleh

stolyaroleh Aug 17, 2017

When readResponses hits an unrecoverable read error on line 435 and 463, it closes this channel by calling broadcastDone, thus signaling to all goroutines that they should shutdown. Since receive on a closed channel does not block, having:

select {
case <-c.done:
   cleanup()
   return
case ...:
   ...
}

in a goroutine will shut it down after broadcastDone.

I am doing a nonblocking select here to prevent c.done from being closed twice, since that will trigger a panic.

Show outdated Hide outdated xgb.go
@@ -366,9 +398,8 @@ func (c *Conn) writeBuffer(buf []byte) error {
if _, err := c.conn.Write(buf); err != nil {
Logger.Printf("A write error is unrecoverable: %s", err)

This comment has been minimized.

@stolyaroleh

stolyaroleh Aug 17, 2017

I wonder if broadcastDone should be called here, too. writeBuffer returns an error which is ignored. Sending another request will likely fail.

@stolyaroleh

stolyaroleh Aug 17, 2017

I wonder if broadcastDone should be called here, too. writeBuffer returns an error which is ignored. Sending another request will likely fail.

This was referenced Sep 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment