Go: time: document that Local must not be modified

[urandom]

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

$ go version
go version go1.13.1 darwin/amd64

Does this issue reproduce with the latest release?

It's not reproducible every time.

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

go env Output

$ go env

GO111MODULE=""

GOARCH="amd64"

GOBIN=""

GOCACHE="$HOME/Library/Caches/go-build"

GOENV="$HOME/Library/Application Support/go/env"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GONOPROXY=""

GONOSUMDB=""

GOOS="darwin"

GOPATH="$HOME/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/usr/local/Cellar/go/1.13.1/libexec"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/usr/local/Cellar/go/1.13.1/libexec/pkg/tool/darwin_amd64"

GCCGO="gccgo"

AR="ar"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD="./backend/go.mod"

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ky/99mqlqrd2rd46chcz3tqmxkw0000gn/T/go-build279362791=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Seems to be reproducible with MAX_PROCS > 2

What did you expect to see?

Nothing

What did you see instead?

WARNING: DATA RACE
Read at 0x000004186410 by goroutine 36:
  time.Now()
      /usr/local/Cellar/go/1.13.1/libexec/src/time/time.go:1100 +0xf5
  time.sendTime()
      /usr/local/Cellar/go/1.13.1/libexec/src/time/sleep.go:137 +0x44

Previous write at 0x000004186410 by main goroutine:
  [failed to restore the stack]

Goroutine 36 (running) created at:
  runtime.(*timersBucket).addtimerLocked()
      /usr/local/Cellar/go/1.13.1/libexec/src/runtime/time.go:169 +0x10d
  github.com/globalsign/mgo.newcoarseTimeProvider.func1()
      $HOME/go/pkg/mod/github.com/globalsign/[email protected]/coarse_time.go:49 +0x50

or

WARNING: DATA RACE
Read at 0x000004186410 by goroutine 13:
  time.Now()
      /usr/local/Cellar/go/1.13.1/libexec/src/time/time.go:1100 +0xf5
  time.sendTime()
      /usr/local/Cellar/go/1.13.1/libexec/src/time/sleep.go:137 +0x44

Previous write at 0x000004186410 by main goroutine:
  [failed to restore the stack]

Goroutine 13 (running) created at:
  runtime.(*timersBucket).addtimerLocked()
      /usr/local/Cellar/go/1.13.1/libexec/src/runtime/time.go:169 +0x10d
  crypto/rand.(*devReader).Read()
      /usr/local/Cellar/go/1.13.1/libexec/src/crypto/rand/rand_unix.go:54 +0x884
  io.ReadAtLeast()
      /usr/local/Cellar/go/1.13.1/libexec/src/io/io.go:310 +0x98
  go.mongodb.org/mongo-driver/bson/primitive.readRandomUint32()
      /usr/local/Cellar/go/1.13.1/libexec/src/io/io.go:329 +0xc7
  go.mongodb.org/mongo-driver/bson/primitive.init()
      $HOME/go/pkg/mod/go.mongodb.org/[email protected]/bson/primitive/objectid.go:34 +0x95

and

WARNING: DATA RACE
Read at 0x00000417df90 by goroutine 12:
  time.Now()
      /usr/local/Cellar/go/1.13.1/libexec/src/time/time.go:1100 +0xf5
  time.sendTime()
      /usr/local/Cellar/go/1.13.1/libexec/src/time/sleep.go:137 +0x44

Previous write at 0x00000417df90 by main goroutine:
  [failed to restore the stack]

Goroutine 12 (running) created at:
  runtime.(*timersBucket).addtimerLocked()
      /usr/local/Cellar/go/1.13.1/libexec/src/runtime/time.go:169 +0x10d
  github.com/golang/glog.(*loggingT).flushDaemon()
      $HOME/go/pkg/mod/github.com/golang/[email protected]/glog.go:882 +0x41

[SchumacherFM]

We can't provide the source code but a compiled go test -c binary for inspections. Downloadable on a private URL.

[urandom]

After figuring out how to bring back the main routine stack, the write occurs in an init function that sets the time.Local to the current location.

[bcmills]

time.go:1100 is this:
https://github.com/golang/go/blob/49073c579e02ad97a1c3e0b645c4cac1a3622fb6/src/time/time.go#L1100

hasMonotonic and nsecShift are constants, and sec, nsec, and mono are all local (unshared) variables, so the race must be on the Local variable.

[bcmills]

@urandom, is anything in your program mutating the time.Local variable?

[urandom]

@bcmills
As mentioned in the previous comment, an init function mutates time.Local.

[bcmills]

As mentioned in the previous comment, an init function mutates time.Local.

Yeah, don't do that. That's an application error, not a bug in the standard library.

As you have seen, packages can (and do) assume that it is safe to spawn goroutines that call time.Now in an init function, but those calls are inherently racy if the program is concurrently mutating the variables that determine the behavior of time.Now.

If you need to customize a program's notion of “local time”, that's what the TZ environment variable is for:
https://github.com/golang/go/blob/fb9af8411eaa3e38d2f72e28a305772f50042657/src/time/zoneinfo_unix.go#L29-L32

[gopherbot]

Change https://golang.org/cl/200457 mentions this issue: time: document that Local and UTC must not be modified

[urandom]

@bcmills, while the doc change would be useful, actually mentioning the TZ env anywhere in the time package would have a greater effect. As it currently stands, even if you mention that one should not modify Local, a user will have no idea what to do to actually change the local TZ.

[bcmills]

Sure, it seems reasonable to mention the use of TZ as part of the doc fix. 🙂

[AndersonQ]

Hi I'm submitting a CL to improve the documentation as is requested in the issue.

However, I'm wondering if Local and UTC should not be changed, and also they are a copy of localLoc and utcLoc, respectively, to protect them from misbehaved clients. Wouldn't be better to have them as functions returning localLoc and utcLoc?

I know, right now it would break the compatibility rule. Nevertheless I'd like to raise this idea. Maybe create another function and recommend to use it instead of Local/UTC

[gopherbot]

Change https://golang.org/cl/200858 mentions this issue: time: improve Local and UTC documentation

[ianlancetaylor]

I agree that it would be nice if there were a way to prevent these variables from being changed. But language style is generally to use variables rather than functions where appropriate. Given that we already have variables, and they do work, I don't see a strong justification for changing to functions.