probes leak two goroutines when closing attach/exec window

[rade]

Step to reproduce:

1. scope launch
2. run docker logs -f weavescope |& grep "goroutine"
3. open http://localhost:4040 in a browser (I used chrome)
4. unhide system containers, click on the weave scope container to bring up the details panel, click on the 'attach' or 'exec' button, then close the "window" that just opened
5. find the scope-app process and dump its threads with kill -QUIT pid
6. count the number of goroutines reported in the above log tail
7. repeat step 4 a few times
8. repeat steps 5 & 6

Observe that the number of goroutines has increased by 2x the number of times step 4 has been repeated.

The leaked goroutines look like this:

goroutine 1067 [chan send]:
github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient.(*Client).hijack.func1.1.1(0xc8204bc230, 0xc820160240)
    /go/src/github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient/client.go:714 +0xc1
github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient.(*Client).hijack.func1.1(0xc8204bc230, 0xc820160240, 0xc820235620, 0xc8201601e0)
    /go/src/github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient/client.go:725 +0x10d
created by github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient.(*Client).hijack.func1
    /go/src/github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient/client.go:725 +0x6ca

goroutine 1000 [chan send]:
github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient.(*Client).hijack.func1.2(0xc821ab2870, 0x7ffac08ab318, 0xc8217ac048, 0xc820cb40c0)
    /go/src/github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient/client.go:733 +0xd7
created by github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient.(*Client).hijack.func1
    /go/src/github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient/client.go:737 +0x3a3

goroutine 1098 [chan receive]:
github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient.(*Client).hijack.func3(0x0, 0x0)
    /go/src/github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient/client.go:765 +0x4e
github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient.waiterFunc.Wait(0xc8204d2700, 0x0, 0x0)
    /go/src/github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient/client.go:632 +0x29
go.(*struct { github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient.closerFunc; github.com/weaveworks/scope/vendor/github.com/fsouza/go-dockerclient.waiterFunc }).Wait(0xc8204d2730, 0x0, 0x0)
    <autogenerated>:23 +0x47
github.com/weaveworks/scope/probe/docker.(*registry).attachContainer.func2(0x7ffac1903938, 0xc8204d2730, 0x7ffac19038a0, 0xc82197aa80)
    /go/src/github.com/weaveworks/scope/probe/docker/controls.go:98 +0x36
created by github.com/weaveworks/scope/probe/docker.(*registry).attachContainer
    /go/src/github.com/weaveworks/scope/probe/docker/controls.go:102 +0x5a1

AFAIK we get one of the first two, and the last.

These goroutines appear to stick around forever - they were still there after ten minutes.

Note that the app does not appear to leak any goroutines when doing this. And no file descriptors appear to leak from either the probe or app.

[rade]

We had ws related leaks in the app before - see #1189. But I could not find any previous reports of leaks in the probe.

[rade]

This patch to go-dockerclient fixes this:

diff --git a/vendor/github.com/fsouza/go-dockerclient/client.go b/vendor/github.com/fsouza/go-dockerclient/client.go
index 3941157..150d4c8 100644
--- a/vendor/github.com/fsouza/go-dockerclient/client.go
+++ b/vendor/github.com/fsouza/go-dockerclient/client.go
@@ -676,7 +676,7 @@ func (c *Client) hijack(method, path string, hijackOptions hijackOptions) (Close
                }
        }

-       errs := make(chan error)
+       errs := make(chan error, 1)
        quit := make(chan struct{})
        go func() {
                clientconn := httputil.NewClientConn(dial, nil)
@@ -690,7 +690,7 @@ func (c *Client) hijack(method, path string, hijackOptions hijackOptions) (Close
                defer rwc.Close()

                errChanOut := make(chan error, 1)
-               errChanIn := make(chan error, 1)
+               errChanIn := make(chan error, 2)
                if hijackOptions.stdout == nil && hijackOptions.stderr == nil {
                        close(errChanOut)
                } else {
@@ -740,14 +740,12 @@ func (c *Client) hijack(method, path string, hijackOptions hijackOptions) (Close
                select {
                case errIn = <-errChanIn:
                case <-quit:
-                       return
                }

                var errOut error
                select {
                case errOut = <-errChanOut:
                case <-quit:
-                       return
                }

                if errIn != nil {

The problem is two-fold:

• there can be two goroutines that attempt to output an item on errIn but the channel only has a capacity of one, so one of the goroutines will get stuck if the closer is invoked (i.e. we hit the <-quit branch), which is why we see one of first two goroutines appear in the goroutine dump. We fix this by increasing the errIn capacity to two.
• the waiter is waiting for input on errs, but invoking the closer ends up terminating the top-level goroutine without outputting on errs, causing the waiter to be stuck. That is the third goroutine we see in the dumps. We fix this by a) removing the returns on <-quit, and b) setting the capacity of errs to one, so that the top-level goroutine doesn't end up blocking if there is no waiter.

@tomwilkie I see you worked on this code in fsouza/go-dockerclient#432. Looks like both issues were introduced there. Would you mind checking my analysis and fix above?

[2opremio]

I am going to create a PR to go-dockerclient with the patch above

[tomwilkie]

TBH I don't see why the two selects can't be merged at the end, which would remove the need for the errChanIn <- nil AFAICT (and probably also need you to get rid of the close(errChanOut).

[rade]

TBH I don't see why the two selects can't be merged at the end

They can't since we need to do one read from errIn and one from errOut.