time: document that Local must not be modified #34814

urandom · 2019-10-10T09:29:05Z

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/mgo@v0.0.0-20181015135952-eeefdecb41b8/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/mongo-driver@v1.1.1/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/glog@v0.0.0-20160126235308-23def4e6c14b/glog.go:882 +0x41

The text was updated successfully, but these errors were encountered:

SchumacherFM · 2019-10-10T09:31:57Z

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

urandom · 2019-10-10T09:38:33Z

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 · 2019-10-10T12:55:46Z

time.go:1100 is this:

go/src/time/time.go

Line 1100 in 49073c5

return Time{hasMonotonic | uint64(sec)<<nsecShift | uint64(nsec), mono, Local}

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 · 2019-10-10T12:56:16Z

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

urandom · 2019-10-10T13:42:58Z

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

bcmills · 2019-10-10T13:54:53Z

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:

go/src/time/zoneinfo_unix.go

Lines 29 to 32 in fb9af84

    
           // consult $TZ to find the time zone to use. 
        
           // no $TZ means use the system default /etc/localtime. 
        
           // $TZ="" means use UTC. 
        
           // $TZ="foo" means use /usr/share/zoneinfo/foo.

gopherbot · 2019-10-10T19:11:14Z

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

urandom · 2019-10-11T06:54:02Z

@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 · 2019-10-11T13:30:36Z

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

AndersonQ · 2019-10-12T11:45:32Z

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 · 2019-10-12T11:46:11Z

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

The existing documentation does not explicitly state clients should not modify their values. Fixes golang#34814 Change-Id: I9b8390545c1dd43dc774b5f7b294257ba4e41ad5

ianlancetaylor · 2019-10-15T22:52:06Z

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.

bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 10, 2019

bcmills changed the title ~~Sporadic data races with time.Time~~ time: sporadic data race on time.Local variable Oct 10, 2019

bcmills changed the title ~~time: sporadic data race on time.Local variable~~ time: document that Local and UTC must not be modified Oct 10, 2019

gopherbot added the Documentation label Oct 10, 2019

bcmills added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Oct 10, 2019

bcmills added this to the Unplanned milestone Oct 10, 2019

bcmills changed the title ~~time: document that Local and UTC must not be modified~~ time: document that Local must not be modified Oct 11, 2019

bcmills mentioned this issue Oct 11, 2019

proposal: os: Stdin, Stdout and Stderr should be interfaces #13473

Open

AndersonQ mentioned this issue Oct 12, 2019

time: improve Local and UTC documentation #34859

Closed

gopherbot closed this as completed in 9bde9b4 Nov 1, 2019

golang locked and limited conversation to collaborators Oct 31, 2020

gopherbot added the FrozenDueToAge label Oct 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

time: document that Local must not be modified #34814

time: document that Local must not be modified #34814

urandom commented Oct 10, 2019 •

edited

SchumacherFM commented Oct 10, 2019

urandom commented Oct 10, 2019

bcmills commented Oct 10, 2019

bcmills commented Oct 10, 2019

urandom commented Oct 10, 2019

bcmills commented Oct 10, 2019 •

edited

gopherbot commented Oct 10, 2019

urandom commented Oct 11, 2019

bcmills commented Oct 11, 2019

AndersonQ commented Oct 12, 2019

gopherbot commented Oct 12, 2019

ianlancetaylor commented Oct 15, 2019

time: document that Local must not be modified #34814

time: document that Local must not be modified #34814

Comments

urandom commented Oct 10, 2019 • edited

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

SchumacherFM commented Oct 10, 2019

urandom commented Oct 10, 2019

bcmills commented Oct 10, 2019

bcmills commented Oct 10, 2019

urandom commented Oct 10, 2019

bcmills commented Oct 10, 2019 • edited

gopherbot commented Oct 10, 2019

urandom commented Oct 11, 2019

bcmills commented Oct 11, 2019

AndersonQ commented Oct 12, 2019

gopherbot commented Oct 12, 2019

ianlancetaylor commented Oct 15, 2019

urandom commented Oct 10, 2019 •

edited

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

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

bcmills commented Oct 10, 2019 •

edited