code review 6855110: net: fix data races on deadline vars

net: fix data races on deadline vars

Fixes issue 4434.

[dave_cheney.net, 2012-11-28 10:13:47]

Hello mikioh.mikioh@gmail.com, bradfitz@golang.org, dvyukov@google.com (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[dvyukov, 2012-11-28 10:29:34]

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/sockopt_posix.go
File src/pkg/net/sockopt_posix.go (left):

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/sockopt_posix.go...
src/pkg/net/sockopt_posix.go:122: func setReadDeadline(fd *netFD, t time.Time)
error {
This was also used for windows. Windows build will be broken with this change.

[dave_cheney.net, 2012-11-28 11:06:39]

On 2012/11/28 10:29:34, dvyukov wrote:
> https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/sockopt_posix.go
> File src/pkg/net/sockopt_posix.go (left):
> 
>
https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/sockopt_posix.go...
> src/pkg/net/sockopt_posix.go:122: func setReadDeadline(fd *netFD, t time.Time)
> error {
> This was also used for windows. Windows build will be broken with this change.

Ok, thanks. I did do a cross compile build to check this, but I will check
again.

[bradfitz, 2012-11-28 14:21:03]

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:44: // read and write deadlines (nsec since 1970 or 0 if
not set)
I'd move the parenthetical part to the deadline type's docs

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:54: type deadline struct {
no need for the struct.  just:

// deadline is an atomically-accessed number of nanoseconds since 1970
// or 0, if no deadline is set.
type deadline int64

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:63: func (d *deadline) set(v int64) {
also add

func (d *deadline) setTime(t time.Time) {
   if t.IsZero() {
      d.set(0)
   } else {
      d.set(t.UnixNano())
   }
}

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:213: t = t - time.Now().UnixNano()
make a new variable for this now.

time.Time is a deadline, which can use variable "t".
a timeout is a time.Duration, which shouldn't use "t".

no need to stay in pre-Go1 int64 world here just because that's when this code
was written.

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:617: if t.IsZero() {
these can be both simplified now with fd.[rw]deadline.setTime(t)

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:635: if err := setReadDeadline(fd, t); err != nil {
unnecessary err check.  I'd just do:

fd.rdeadline.setTime(t)
fd.wdeadline.setTime(t)
return nil

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/sock_posix.go#ne...
src/pkg/net/sock_posix.go:61: fd.wdeadline.set(deadline.UnixNano())
setTime

could drop the if too. up to you. adds an atomic op per connect, but that's
probably noise.

[brainman, 2012-11-29 04:17:19]

On 2012/11/28 11:06:39, dfc wrote:
> ...
> ... I did do a cross compile build to check this, ...

I cannot build test now:

# GOOS=windows go test -c
# net
./net.go:161: undefined: setDeadline
./net.go:169: undefined: setReadDeadline
./net.go:177: undefined: setWriteDeadline
./sock_posix.go:61: fd.wdeadline.set undefined (type int64 has no field or
method set)
./sock_posix.go:69: fd.wdeadline.set undefined (type int64 has no field or
method set)
./tcpsock_posix.go:272: undefined: setDeadline
./unixsock_posix.go:328: undefined: setDeadline

Alex

[dave_cheney.net, 2012-11-29 04:19:15]

Thanks for your comments. I'm addressing them and will post another patch set
this evening.

dmitry/alex - I will fix the windows/setDeadline breakage - I don't know why my
crosscompile script didn't pick this up, maybe it isn't actually cross compiling
:(

bradfitz - I have one more point in your comments to address then I will post a
new patch set.

[brainman, 2012-11-29 04:21:56]

On 2012/11/29 04:19:15, dfc wrote:
> ... I don't know why my
> crosscompile script didn't pick this up, maybe it isn't actually cross
compiling
> :(
> 

Perhaps your script compiles and install windows packages, but you are not
building tests. You could do both.

Alex

[dave_cheney.net, 2012-11-29 06:31:36]

Thank you for your comments and your testing. In order to resolve the changes to
sockopt_posix.go I have converted fd_windows.go to use a deadline type. Other
solutions exist, but they probably mean more code duplication. If it turns out
that the same race exists on Windows as it does in Unix, then resolving it would
be simple.

// TODO(dfc) I'd be interested in merging the common fields and methods from
fd_unix.go:netFD and fd_windows.go:netFD

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:44: // read and write deadlines (nsec since 1970 or 0 if
not set)
On 2012/11/28 14:21:03, bradfitz wrote:
> I'd move the parenthetical part to the deadline type's docs

Done.

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:54: type deadline struct {
On 2012/11/28 14:21:03, bradfitz wrote:
> no need for the struct.  just:
> 
> // deadline is an atomically-accessed number of nanoseconds since 1970
> // or 0, if no deadline is set.
> type deadline int64

Done. Originally having this as a struct allowed me to find all the places in
the code (like sendfile_linux.go) where fd.{r,w}deadline was being accessed.

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:63: func (d *deadline) set(v int64) {
On 2012/11/28 14:21:03, bradfitz wrote:
> also add
> 
> func (d *deadline) setTime(t time.Time) {
>    if t.IsZero() {
>       d.set(0)
>    } else {
>       d.set(t.UnixNano())
>    }
> }

Done.

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:213: t = t - time.Now().UnixNano()
On 2012/11/28 14:21:03, bradfitz wrote:
> make a new variable for this now.
> 
> time.Time is a deadline, which can use variable "t".
> a timeout is a time.Duration, which shouldn't use "t".
> 
> no need to stay in pre-Go1 int64 world here just because that's when this code
> was written.

Actually the logic is a little tricky, t is use later int the method, i've
renamed t to deadline, PTAL.

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:617: if t.IsZero() {
On 2012/11/28 14:21:03, bradfitz wrote:
> these can be both simplified now with fd.[rw]deadline.setTime(t)

Done. I have added a TODO to see if can be further improved.

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:635: if err := setReadDeadline(fd, t); err != nil {
On 2012/11/28 14:21:03, bradfitz wrote:
> unnecessary err check.  I'd just do:
> 
> fd.rdeadline.setTime(t)
> fd.wdeadline.setTime(t)
> return nil

Done, see above, I want to make this simpler in general

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/sockopt_posix.go
File src/pkg/net/sockopt_posix.go (left):

https://codereview.appspot.com/6855110/diff/8001/src/pkg/net/sockopt_posix.go...
src/pkg/net/sockopt_posix.go:122: func setReadDeadline(fd *netFD, t time.Time)
error {
On 2012/11/28 10:29:34, dvyukov wrote:
> This was also used for windows. Windows build will be broken with this change.

Done.

[dave_cheney.net, 2012-11-29 06:32:17]

Hello mikioh.mikioh@gmail.com, bradfitz@golang.org, dvyukov@google.com,
alex.brainman@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-11-29 06:41:45]

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:223: var deadline = s.deadline
:=

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:225: deadline -= time.Now().UnixNano()
but now it's not a deadline, right?  WaitFD takes a timeout or zero?

yeah, looking at fd_linux.go, that goes right to epoll_wait, which take a
timeout.

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/sock_posix.go#ne...
src/pkg/net/sock_posix.go:61: fd.wdeadline.set(deadline.UnixNano())
setTime ?

[bradfitz, 2012-11-29 06:44:08]

LGTM

None of those are real problems.  I just don't like the name of that
variable, which is a timeout and not a deadline.

I didn't review the Windows changes as closely.


On Wed, Nov 28, 2012 at 10:41 PM, <bradfitz@golang.org> wrote:

>
> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>
net/fd_unix.go<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go>
> File src/pkg/net/fd_unix.go (right):
>
> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>
net/fd_unix.go#newcode223<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go#newcode223>
> src/pkg/net/fd_unix.go:223: var deadline = s.deadline
> :=
>
> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>
net/fd_unix.go#newcode225<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go#newcode225>
> src/pkg/net/fd_unix.go:225: deadline -= time.Now().UnixNano()
> but now it's not a deadline, right?  WaitFD takes a timeout or zero?
>
> yeah, looking at fd_linux.go, that goes right to epoll_wait, which take
> a timeout.
>
> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>
net/sock_posix.go<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/sock_posix.go>
> File src/pkg/net/sock_posix.go (right):
>
> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>
net/sock_posix.go#newcode61<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/sock_posix.go#newcode61>
> src/pkg/net/sock_posix.go:61: fd.wdeadline.set(deadline.**UnixNano())
> setTime ?
>
>
https://codereview.appspot.**com/6855110/<https://codereview.appspot.com/6855...
>

[bradfitz, 2012-11-29 06:44:51]

LGTM

Nevermind, the Windows changes are identical to the POSIX ones, which is
why I didn't remember them.

On Wed, Nov 28, 2012 at 10:44 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> LGTM
>
> None of those are real problems.  I just don't like the name of that
> variable, which is a timeout and not a deadline.
>
> I didn't review the Windows changes as closely.
>
>
> On Wed, Nov 28, 2012 at 10:41 PM, <bradfitz@golang.org> wrote:
>
>>
>> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>>
net/fd_unix.go<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go>
>> File src/pkg/net/fd_unix.go (right):
>>
>> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>>
net/fd_unix.go#newcode223<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go#newcode223>
>> src/pkg/net/fd_unix.go:223: var deadline = s.deadline
>> :=
>>
>> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>>
net/fd_unix.go#newcode225<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go#newcode225>
>> src/pkg/net/fd_unix.go:225: deadline -= time.Now().UnixNano()
>> but now it's not a deadline, right?  WaitFD takes a timeout or zero?
>>
>> yeah, looking at fd_linux.go, that goes right to epoll_wait, which take
>> a timeout.
>>
>> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>>
net/sock_posix.go<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/sock_posix.go>
>> File src/pkg/net/sock_posix.go (right):
>>
>> https://codereview.appspot.**com/6855110/diff/7012/src/pkg/**
>>
net/sock_posix.go#newcode61<https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/sock_posix.go#newcode61>
>> src/pkg/net/sock_posix.go:61: fd.wdeadline.set(deadline.**UnixNano())
>> setTime ?
>>
>>
https://codereview.appspot.**com/6855110/<https://codereview.appspot.com/6855...
>>
>
>

[dave_cheney.net, 2012-11-29 06:50:06]

Thanks Brad. I need to wait for Alex and Mikio to confirm I haven't broken the
freebsd and windows builds.

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:223: var deadline = s.deadline
On 2012/11/29 06:41:46, bradfitz wrote:
> :=

Done.

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:225: deadline -= time.Now().UnixNano()
On 2012/11/29 06:41:46, bradfitz wrote:
> but now it's not a deadline, right?  WaitFD takes a timeout or zero?
> 
> yeah, looking at fd_linux.go, that goes right to epoll_wait, which take a
> timeout.
> 

I'd really like to leave this as untouched as possible for the moment, i've
reverted back to the previous patch set and added a TODO.

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/6855110/diff/7012/src/pkg/net/sock_posix.go#ne...
src/pkg/net/sock_posix.go:61: fd.wdeadline.set(deadline.UnixNano())
On 2012/11/29 06:41:46, bradfitz wrote:
> setTime ?

Done.

[mikio, 2012-11-29 12:39:47]

LGTM

https://codereview.appspot.com/6855110/diff/7014/src/pkg/net/fd_posix_test.go
File src/pkg/net/fd_posix_test.go (right):

https://codereview.appspot.com/6855110/diff/7014/src/pkg/net/fd_posix_test.go...
src/pkg/net/fd_posix_test.go:14: var deadlineSetTimeTests = []struct {
deadlineSetValueTests?

[mikio, 2012-11-29 13:25:53]

> LGTM

Hold on, sendfile_freebsd.go needs a twiddle too.

[dave_cheney.net, 2012-11-29 21:58:05]

Hello mikioh.mikioh@gmail.com, bradfitz@golang.org, dvyukov@google.com,
alex.brainman@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-11-29 21:58:30]

Thanks for your comments, I had missed hg add'ing sendfile_freebsd.go. PTAL.

https://codereview.appspot.com/6855110/diff/7014/src/pkg/net/fd_posix_test.go
File src/pkg/net/fd_posix_test.go (right):

https://codereview.appspot.com/6855110/diff/7014/src/pkg/net/fd_posix_test.go...
src/pkg/net/fd_posix_test.go:14: var deadlineSetTimeTests = []struct {
On 2012/11/29 12:39:47, mikio wrote:
> deadlineSetValueTests?

I've renamed the test, it used to be testing the round trip from set -> value,
but setTime -> value was more useful.

[mikio, 2012-11-29 22:37:05]

LGTM

[bradfitz, 2012-11-29 22:47:15]

https://codereview.appspot.com/6855110/diff/12008/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6855110/diff/12008/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:225: // TODO(dfc) is this a time, or a duration
s.deadline is a duration and what you pass to WaitFD is a timeout.

the confusion is that this code uses a variable which starts as one type
(deadline) and changes to another type (by subtracting time.Now() from it)

I would rewrite this like this:

var timeout int64 // nsec, or 0 for none
if s.deadline > 0 {
    timeout = s.deadline - unix.Now().UnixNano()
    if timeout <= 0 {
            ...
    }
}
fd, mode, err := ....

[bradfitz, 2012-11-29 22:48:18]

https://codereview.appspot.com/6855110/diff/12008/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6855110/diff/12008/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:225: // TODO(dfc) is this a time, or a duration
On 2012/11/29 22:47:15, bradfitz wrote:
> s.deadline is a duration and what you pass to WaitFD is a timeout.

ARGH, typo, sorry.

s.deadline is a deadline (a time.Time, absolute point in time)

t is a timeout (a duration)

Sorry for the typo.  Not helpful.

[brainman, 2012-11-29 23:09:39]

LGTM

[dave_cheney.net, 2012-11-29 23:40:36]

ok, all done. Going to smoke test this a bit more, then submit after work
tonight.

https://codereview.appspot.com/6855110/diff/12008/src/pkg/net/fd_unix.go
File src/pkg/net/fd_unix.go (right):

https://codereview.appspot.com/6855110/diff/12008/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:225: // TODO(dfc) is this a time, or a duration
On 2012/11/29 22:48:18, bradfitz wrote:
> On 2012/11/29 22:47:15, bradfitz wrote:
> > s.deadline is a duration and what you pass to WaitFD is a timeout.
> 
> ARGH, typo, sorry.
> 
> s.deadline is a deadline (a time.Time, absolute point in time)
> 
> t is a timeout (a duration)
> 
> Sorry for the typo.  Not helpful.

Done. I think

[dave_cheney.net, 2012-11-29 23:41:01]

Hello mikioh.mikioh@gmail.com, bradfitz@golang.org, dvyukov@google.com,
alex.brainman@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-11-29 23:41:35]

LGTM

[dave_cheney.net, 2012-11-30 07:27:05]

*** Submitted as https://code.google.com/p/go/source/detail?r=869253ef7009 ***

net: fix data races on deadline vars

Fixes issue 4434.

R=mikioh.mikioh, bradfitz, dvyukov, alex.brainman
CC=golang-dev
https://codereview.appspot.com/6855110