Skip to content
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

time: document that Local must not be modified #34814

Open
urandom opened this issue Oct 10, 2019 · 12 comments

Comments

@urandom
Copy link

commented Oct 10, 2019

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
@SchumacherFM

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

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

@urandom

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

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

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

time.go:1100 is this:

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

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

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

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

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

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

@bcmills

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

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:

// 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.

@bcmills 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
@bcmills bcmills added this to the Unplanned milestone Oct 10, 2019
@gopherbot

This comment has been minimized.

Copy link

commented Oct 10, 2019

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

@urandom

This comment has been minimized.

Copy link
Author

commented Oct 11, 2019

@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

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

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

@bcmills 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
@AndersonQ

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2019

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

This comment has been minimized.

Copy link

commented Oct 12, 2019

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

AndersonQ added a commit to AndersonQ/go that referenced this issue Oct 12, 2019
The existing documentation does not explicitly state clients should not modify their values.

Fixes golang#34814

Change-Id: I9b8390545c1dd43dc774b5f7b294257ba4e41ad5
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.