Issue 6845091: code review 6845091: net: add deadline prolongation test (Closed)
|
Created:
11 years, 5 months ago by dvyukov Modified:
11 years, 5 months ago Reviewers:
CC:
golang-dev, dave_cheney.net, bradfitz, rsc Visibility:
Public. |
Descriptionnet: add deadline prolongation test
Currently the test exposes data races on deadline vars.
Patch Set 1 #Patch Set 2 : diff -r d351a7cc9ca9 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r d351a7cc9ca9 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r d351a7cc9ca9 https://go.googlecode.com/hg/ #
Total comments: 4
Patch Set 5 : diff -r d351a7cc9ca9 https://go.googlecode.com/hg/ #
Total comments: 9
Patch Set 6 : diff -r 03a6b8c9c396 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 7 : diff -r 03a6b8c9c396 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 8 : diff -r 03a6b8c9c396 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 9 : diff -r 101e647105e8 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 10 : diff -r 4455310240f7 https://dvyukov%40google.com@code.google.com/p/go/ #MessagesTotal messages: 14
Hello golang-dev@googlegroups.com (cc: bradfitz@golang.org, dave@cheney.net), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
Thank you, I am sure this will set off the race detector. https://codereview.appspot.com/6845091/diff/3/src/pkg/net/timeout_test.go File src/pkg/net/timeout_test.go (right): https://codereview.appspot.com/6845091/diff/3/src/pkg/net/timeout_test.go#new... src/pkg/net/timeout_test.go:589: c, err := DialTCP("tcp", nil, ln.Addr().(*TCPAddr)) Does this need to be DialTCP, all net.Conn implementations implement Set{Read,Write,}Deadline. https://codereview.appspot.com/6845091/diff/3/src/pkg/net/timeout_test.go#new... src/pkg/net/timeout_test.go:607: s.SetDeadline(time.Now().Add(time.Hour)) Did you mean SetReadDeadline ?
Sign in to reply to this message.
Yes, it is:
==================
WARNING: DATA RACE
Write by goroutine 4:
net.setReadDeadline()
src/pkg/net/sockopt_posix.go:126 +0xdf
net.setDeadline()
src/pkg/net/sockopt_posix.go:141 +0x4e
net.(*conn).SetDeadline()
src/pkg/net/net.go:161 +0xe3
net.func·058()
src/pkg/net/timeout_test.go:607 +0x1ca
Previous read by goroutine 5:
net.(*netFD).Read()
src/pkg/net/fd_unix.go:426 +0xfb
net.(*conn).Read()
src/pkg/net/net.go:121 +0x101
net.func·059()
src/pkg/net/timeout_test.go:613 +0xbf
Goroutine 4 (running) created at:
net.TestProlongTimeout()
src/pkg/net/timeout_test.go:609 +0x517
testing.tRunner()
src/pkg/testing/testing.go:301 +0xe8
Goroutine 5 (running) created at:
net.TestProlongTimeout()
src/pkg/net/timeout_test.go:619 +0x531
testing.tRunner()
src/pkg/testing/testing.go:301 +0xe8
==================
==================
WARNING: DATA RACE
Read by goroutine 4:
net.(*netFD).Write()
src/pkg/net/fd_unix.go:533 +0x12e
net.(*conn).Write()
src/pkg/net/net.go:129 +0x101
net.func·058()
src/pkg/net/timeout_test.go:603 +0x106
Previous write by goroutine 5:
net.setWriteDeadline()
src/pkg/net/sockopt_posix.go:135 +0xdf
net.setDeadline()
src/pkg/net/sockopt_posix.go:144 +0x9c
net.(*conn).SetDeadline()
src/pkg/net/net.go:161 +0xe3
net.func·059()
src/pkg/net/timeout_test.go:617 +0x168
Goroutine 4 (running) created at:
net.TestProlongTimeout()
src/pkg/net/timeout_test.go:609 +0x517
testing.tRunner()
src/pkg/testing/testing.go:301 +0xe8
Goroutine 5 (running) created at:
net.TestProlongTimeout()
src/pkg/net/timeout_test.go:619 +0x531
testing.tRunner()
src/pkg/testing/testing.go:301 +0xe8
==================
https://codereview.appspot.com/6845091/diff/3/src/pkg/net/timeout_test.go
File src/pkg/net/timeout_test.go (right):
https://codereview.appspot.com/6845091/diff/3/src/pkg/net/timeout_test.go#new...
src/pkg/net/timeout_test.go:589: c, err := DialTCP("tcp", nil,
ln.Addr().(*TCPAddr))
On 2012/11/25 11:09:10, dfc wrote:
> Does this need to be DialTCP, all net.Conn implementations implement
> Set{Read,Write,}Deadline.
Done.
https://codereview.appspot.com/6845091/diff/3/src/pkg/net/timeout_test.go#new...
src/pkg/net/timeout_test.go:607: s.SetDeadline(time.Now().Add(time.Hour))
On 2012/11/25 11:09:10, dfc wrote:
> Did you mean SetReadDeadline ?
No, I don't.
This is pretty synthetic anyway. Currently it exposes races on both read and
write deadline vars.
Sign in to reply to this message.
LGTM. Thanks, we've been making a lot of changes to net over the long weekend, please wait for others to review our work before submitting.
Sign in to reply to this message.
No major comments. https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go File src/pkg/net/timeout_test.go (right): https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go... src/pkg/net/timeout_test.go:589: c, err := Dial("tcp", ln.Addr().String()) you're assuming that this doesn't block (that the kernel buffers at least one accept until your Accept on line 594 below), which is probably safe on all current platforms, but not something that the Go API guarantees. https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go... src/pkg/net/timeout_test.go:601: buf := [4096]byte{} var buf [4096]byte seems more natural https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go... src/pkg/net/timeout_test.go:607: s.SetDeadline(time.Now().Add(time.Hour)) do you mean to set both deadlines, or just SetWriteDeadline? https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go... src/pkg/net/timeout_test.go:617: s.SetDeadline(time.Now().Add(time.Hour)) do you mean to set both deadlines, or just SetReadDeadline? https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go... src/pkg/net/timeout_test.go:621: buf := [1]byte{} var buf [1]byte
Sign in to reply to this message.
https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go File src/pkg/net/timeout_test.go (right): https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go... src/pkg/net/timeout_test.go:589: c, err := Dial("tcp", ln.Addr().String()) On 2012/11/26 00:46:41, bradfitz wrote: > you're assuming that this doesn't block (that the kernel buffers at least one > accept until your Accept on line 594 below), which is probably safe on all > current platforms, but not something that the Go API guarantees. Done. https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go... src/pkg/net/timeout_test.go:601: buf := [4096]byte{} On 2012/11/26 00:46:41, bradfitz wrote: > var buf [4096]byte > > seems more natural Done. https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go... src/pkg/net/timeout_test.go:607: s.SetDeadline(time.Now().Add(time.Hour)) On 2012/11/26 00:46:41, bradfitz wrote: > do you mean to set both deadlines, or just SetWriteDeadline? I mean to set both. It stresses setup of the same type of deadline from several goroutines. I can imagine it happens if e.g. you process each incoming command in a separate goroutine; if you receive several keepalive commands in short succession, you can end up calling SetDeadline() concurrently. https://codereview.appspot.com/6845091/diff/11001/src/pkg/net/timeout_test.go... src/pkg/net/timeout_test.go:621: buf := [1]byte{} On 2012/11/26 00:46:41, bradfitz wrote: > var buf [1]byte Done.
Sign in to reply to this message.
LGTM Okay I guess. It seems like broken code but I agree it shouldn't trigger races in package net.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=d2b512689ae1 *** net: add deadline prolongation test Currently the test exposes data races on deadline vars. R=golang-dev, dave, bradfitz, rsc CC=golang-dev http://codereview.appspot.com/6845091
Sign in to reply to this message.
Wasn't this test expected to fail ? On 27 Nov 2012 05:30, <dvyukov@google.com> wrote: > *** Submitted as > http://code.google.com/p/go/**source/detail?r=d2b512689ae1<http://code.google... > > net: add deadline prolongation test > Currently the test exposes data races on deadline vars. > > R=golang-dev, dave, bradfitz, rsc > CC=golang-dev > http://codereview.appspot.com/**6845091<http://codereview.appspot.com/6845091> > > > http://codereview.appspot.com/**6845091/<http://codereview.appspot.com/6845091/> >
Sign in to reply to this message.
Good question. Fails on my machine. On Mon, Nov 26, 2012 at 10:48 PM, Dave Cheney <dave@cheney.net> wrote: > Wasn't this test expected to fail ? > > On 27 Nov 2012 05:30, <dvyukov@google.com> wrote: >> >> *** Submitted as >> http://code.google.com/p/go/source/detail?r=d2b512689ae1 *** >> >> net: add deadline prolongation test >> Currently the test exposes data races on deadline vars. >> >> R=golang-dev, dave, bradfitz, rsc >> CC=golang-dev >> http://codereview.appspot.com/6845091 >> >> >> http://codereview.appspot.com/6845091/
Sign in to reply to this message.
booa-ga-ga My buildbot script is: $ cat test.sh #!/bin/sh export GOROOT="$(cd .. && pwd)" export PATH=$GOROOT/bin:$PATH ./make.bash go install -race std go test -race -short std go test -race -run=nothingplease -bench=.* -benchtime=.1s -cpu=4 std I guess I need to add set -x ... On Mon, Nov 26, 2012 at 10:56 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > Good question. Fails on my machine. > > On Mon, Nov 26, 2012 at 10:48 PM, Dave Cheney <dave@cheney.net> wrote: >> Wasn't this test expected to fail ? >> >> On 27 Nov 2012 05:30, <dvyukov@google.com> wrote: >>> >>> *** Submitted as >>> http://code.google.com/p/go/source/detail?r=d2b512689ae1 *** >>> >>> net: add deadline prolongation test >>> Currently the test exposes data races on deadline vars. >>> >>> R=golang-dev, dave, bradfitz, rsc >>> CC=golang-dev >>> http://codereview.appspot.com/6845091 >>> >>> >>> http://codereview.appspot.com/6845091/
Sign in to reply to this message.
Did you mean -e ? On 27 Nov 2012 06:02, "Dmitry Vyukov" <dvyukov@google.com> wrote: > booa-ga-ga > > My buildbot script is: > > $ cat test.sh > #!/bin/sh > export GOROOT="$(cd .. && pwd)" > export PATH=$GOROOT/bin:$PATH > ./make.bash > go install -race std > go test -race -short std > go test -race -run=nothingplease -bench=.* -benchtime=.1s -cpu=4 std > > I guess I need to add set -x ... > > > > On Mon, Nov 26, 2012 at 10:56 PM, Dmitry Vyukov <dvyukov@google.com> > wrote: > > Good question. Fails on my machine. > > > > On Mon, Nov 26, 2012 at 10:48 PM, Dave Cheney <dave@cheney.net> wrote: > >> Wasn't this test expected to fail ? > >> > >> On 27 Nov 2012 05:30, <dvyukov@google.com> wrote: > >>> > >>> *** Submitted as > >>> http://code.google.com/p/go/source/detail?r=d2b512689ae1 *** > >>> > >>> net: add deadline prolongation test > >>> Currently the test exposes data races on deadline vars. > >>> > >>> R=golang-dev, dave, bradfitz, rsc > >>> CC=golang-dev > >>> http://codereview.appspot.com/6845091 > >>> > >>> > >>> http://codereview.appspot.com/6845091/ >
Sign in to reply to this message.
OK, I better not send out nor commit any change lists today... On Mon, Nov 26, 2012 at 11:04 PM, Dave Cheney <dave@cheney.net> wrote: > Did you mean -e ? > > On 27 Nov 2012 06:02, "Dmitry Vyukov" <dvyukov@google.com> wrote: >> >> booa-ga-ga >> >> My buildbot script is: >> >> $ cat test.sh >> #!/bin/sh >> export GOROOT="$(cd .. && pwd)" >> export PATH=$GOROOT/bin:$PATH >> ./make.bash >> go install -race std >> go test -race -short std >> go test -race -run=nothingplease -bench=.* -benchtime=.1s -cpu=4 std >> >> I guess I need to add set -x ... >> >> >> >> On Mon, Nov 26, 2012 at 10:56 PM, Dmitry Vyukov <dvyukov@google.com> >> wrote: >> > Good question. Fails on my machine. >> > >> > On Mon, Nov 26, 2012 at 10:48 PM, Dave Cheney <dave@cheney.net> wrote: >> >> Wasn't this test expected to fail ? >> >> >> >> On 27 Nov 2012 05:30, <dvyukov@google.com> wrote: >> >>> >> >>> *** Submitted as >> >>> http://code.google.com/p/go/source/detail?r=d2b512689ae1 *** >> >>> >> >>> net: add deadline prolongation test >> >>> Currently the test exposes data races on deadline vars. >> >>> >> >>> R=golang-dev, dave, bradfitz, rsc >> >>> CC=golang-dev >> >>> http://codereview.appspot.com/6845091 >> >>> >> >>> >> >>> http://codereview.appspot.com/6845091/
Sign in to reply to this message.
OK, great, fails now: http://build.golang.org/log/aac207a4038012b0244c57d7bffc01591c4b6900 On Mon, Nov 26, 2012 at 11:07 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > OK, I better not send out nor commit any change lists today... > > On Mon, Nov 26, 2012 at 11:04 PM, Dave Cheney <dave@cheney.net> wrote: >> Did you mean -e ? >> >> On 27 Nov 2012 06:02, "Dmitry Vyukov" <dvyukov@google.com> wrote: >>> >>> booa-ga-ga >>> >>> My buildbot script is: >>> >>> $ cat test.sh >>> #!/bin/sh >>> export GOROOT="$(cd .. && pwd)" >>> export PATH=$GOROOT/bin:$PATH >>> ./make.bash >>> go install -race std >>> go test -race -short std >>> go test -race -run=nothingplease -bench=.* -benchtime=.1s -cpu=4 std >>> >>> I guess I need to add set -x ... >>> >>> >>> >>> On Mon, Nov 26, 2012 at 10:56 PM, Dmitry Vyukov <dvyukov@google.com> >>> wrote: >>> > Good question. Fails on my machine. >>> > >>> > On Mon, Nov 26, 2012 at 10:48 PM, Dave Cheney <dave@cheney.net> wrote: >>> >> Wasn't this test expected to fail ? >>> >> >>> >> On 27 Nov 2012 05:30, <dvyukov@google.com> wrote: >>> >>> >>> >>> *** Submitted as >>> >>> http://code.google.com/p/go/source/detail?r=d2b512689ae1 *** >>> >>> >>> >>> net: add deadline prolongation test >>> >>> Currently the test exposes data races on deadline vars. >>> >>> >>> >>> R=golang-dev, dave, bradfitz, rsc >>> >>> CC=golang-dev >>> >>> http://codereview.appspot.com/6845091 >>> >>> >>> >>> >>> >>> http://codereview.appspot.com/6845091/
Sign in to reply to this message.
|
