x/net/context: go vet fails because of copied Mutex

[bernerdschaefer]

1. What version of Go are you using (go version)?

go version go1.6 linux/amd64

1. What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

1. What did you do?
   If possible, provide a recipe for reproducing the error.
   A complete runnable program is good.
   A link on play.golang.org is best.

Ran go vet ./... on a project with golang.org/x/net/context/context.go in vendor/.

Also tried running go vet on a fresh clone of https://github.com/golang/net.

1. What did you expect to see?

go vet to exit with status 0.

1. What did you see instead?

context.go:218: newCancelCtx returns lock by value: context.cancelCtx contains sync.Mutex
exit status 1

[bernerdschaefer]

The code is correct, but tricky in its use of values and references to maintain the Mutex. A tentative change like this appears to also be correct, passes the tests, and also passes go vet:

diff --git a/context/context.go b/context/context.go
index 77b64d0..19235cf 100644
--- a/context/context.go
+++ b/context/context.go
@@ -210,13 +210,13 @@ type CancelFunc func()
 // call cancel as soon as the operations running in this Context complete.
 func WithCancel(parent Context) (ctx Context, cancel CancelFunc) {
        c := newCancelCtx(parent)
-       propagateCancel(parent, &c)
-       return &c, func() { c.cancel(true, Canceled) }
+       propagateCancel(parent, c)
+       return c, func() { c.cancel(true, Canceled) }
 }

 // newCancelCtx returns an initialized cancelCtx.
-func newCancelCtx(parent Context) cancelCtx {
-       return cancelCtx{
+func newCancelCtx(parent Context) *cancelCtx {
+       return &cancelCtx{
                Context: parent,
                done:    make(chan struct{}),
        }
@@ -259,7 +259,7 @@ func parentCancelCtx(parent Context) (*cancelCtx, bool) {
                case *cancelCtx:
                        return c, true
                case *timerCtx:
-                       return &c.cancelCtx, true
+                       return c.cancelCtx, true
                case *valueCtx:
                        parent = c.Context
                default:
@@ -377,7 +377,7 @@ func WithDeadline(parent Context, deadline time.Time) (Context, CancelFunc) {
 // implement Done and Err.  It implements cancel by stopping its timer then
 // delegating to cancelCtx.cancel.
 type timerCtx struct {
-       cancelCtx
+       *cancelCtx
        timer *time.Timer // Under cancelCtx.mu.

        deadline time.Time

I'm happy to put this into a CL.

[valyala]

The related report on tip go sources using go vet from tip:

go vet ./src/... 2>&1 | grep ' lock ' 
src/crypto/tls/tls.go:138: assignment copies lock value to c: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/conn_test.go:127: assignment copies lock value to serverConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/conn_test.go:231: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/conn_test.go:237: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/conn_test.go:243: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_client_test.go:361: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_client_test.go:397: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_client_test.go:543: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_client_test.go:564: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_client_test.go:588: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:133: assignment copies lock value to serverConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:150: assignment copies lock value to serverConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:175: assignment copies lock value to serverConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:268: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:677: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:693: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:714: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:749: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:771: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:789: assignment copies lock value to serverConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:808: assignment copies lock value to serverConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:836: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:846: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:885: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/handshake_server_test.go:998: assignment copies lock value to config: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/tls_test.go:238: assignment copies lock value to serverConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/tls_test.go:248: assignment copies lock value to clientConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/tls_test.go:292: assignment copies lock value to serverConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/tls_test.go:301: assignment copies lock value to clientConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/tls_test.go:391: assignment copies lock value to serverConfig: tls.Config contains sync.Once contains sync.Mutex
src/crypto/tls/tls_test.go:411: assignment copies lock value to clientConfig: tls.Config contains sync.Once contains sync.Mutex
src/go/internal/gcimporter/bimport.go:286: assignment copies lock value to *t: go/types.Struct contains sync.Once contains sync.Mutex
src/net/http/h2_bundle.go:4858: assignment copies lock value to *cfg: crypto/tls.Config contains sync.Once contains sync.Mutex
src/net/http/httptest/server.go:119: assignment copies lock value to *s.TLS: crypto/tls.Config contains sync.Once contains sync.Mutex
src/regexp/regexp.go:112: assignment copies lock value to r: regexp.Regexp contains sync.Mutex

[dsymonds]

I've been told by @rsc in the past that copying a mutex that has ever been used is verboten, and at least src/regexp/regexp.go:112 is doing that (I didn't check the others). There's a bunch of things that look like they need fixing here.

newCancelCtx (from x/net/context/context.go:117) should probably also be changed to return *cancelCtx instead of the value type. I can't see a good reason for that to return the value; the rest of that file uses it by pointer.

[gopherbot]

CL https://golang.org/cl/20840 mentions this issue.

[gopherbot]

CL https://golang.org/cl/20841 mentions this issue.

[mholt]

Hmm, I'm seeing something very similar from a failing CI job on Go tip:

$ go vet ./...
caddy/config.go:194: function call copies lock value: sync.Once contains sync.Mutex
caddy/config_test.go:114: function call copies lock value: sync.Once contains sync.Mutex
caddy/config_test.go:115: function call copies lock value: sync.Once contains sync.Mutex

Relevant lines of code are:

onces[dir.name] = new(sync.Once)

map[string]*sync.Once{
    "dummy":  new(sync.Once),
    "dummy2": new(sync.Once),
}

Is new() really a function call that is copying a lock value? It wasn't as of yesterday, and doesn't according to Go 1.6's go vet test. Anyway, just checking to see if this is related.

[bernerdschaefer]

@mholt I think that is not related, as x/net/context legitimately had a function which returned a lock by value: golang/net@9d8ef8d

[bernerdschaefer]

@dsymonds Thanks for getting a fix for this! I think golang/net@9d8ef8d resolves the original issue. Should I close this?

[mholt]

@bernerdschaefer Alrighty, I'll open a new issue then.

[valyala]

Is new() really a function call that is copying a lock value?

This looks like a bug in go vet. I'll look into it tomorrow unless somebody fix it today.

[valyala]

@mholt , @bernerdschaefer , I prepared the CL, which should properly handle new(sync.Mutex) case in go vet.

[mholt]

Cool, thanks, does that cover new(sync.Once) too, then? (Issue: #14994)

[gopherbot]

CL https://golang.org/cl/21222 mentions this issue.