code review 6850070: net: add unit tests for read/write deadlines

net: add unit tests for read/write deadlines
The tests verify that deadlines are "persistent",
read/write deadlines do not interfere, can be reset,
read deadline can be set with both SetDeadline()
and SetReadDeadline(), etc.

[dvyukov, 2012-11-18 14:42:08]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-11-20 02:05:36]

https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go
File src/pkg/net/timeout_test.go (right):

https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:26: tl.SetDeadline(time.Now())
time.Now().Add(-1 * time.Second) to be clear?

https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:49: t.Fatalf("Expected Accept() to not return, but
it retrned with %v\n", err)
typo: returned

https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:90: c.SetReadDeadline(time.Time{})
noDeadline

https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:100: t.Fatalf("Expected Read() to not return, but it
retrned with %v\n", err)
returned

https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:143: c.SetWriteDeadline(time.Time{})
noDeadline

https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:157: t.Fatalf("Expected Write() to not return, but
it retrned with %v\n", err)
returned

[dvyukov, 2012-11-22 09:22:30]

On 2012/11/20 02:05:36, bradfitz wrote:
> https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go
> File src/pkg/net/timeout_test.go (right):
> 
>
https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:26: tl.SetDeadline(time.Now())
> time.Now().Add(-1 * time.Second) to be clear?
> 
>
https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:49: t.Fatalf("Expected Accept() to not return, but
> it retrned with %v\n", err)
> typo: returned
> 
>
https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:90: c.SetReadDeadline(time.Time{})
> noDeadline
> 
>
https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:100: t.Fatalf("Expected Read() to not return, but
it
> retrned with %v\n", err)
> returned
> 
>
https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:143: c.SetWriteDeadline(time.Time{})
> noDeadline
> 
>
https://codereview.appspot.com/6850070/diff/7001/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:157: t.Fatalf("Expected Write() to not return, but
> it retrned with %v\n", err)
> returned

All Done. PTAL.

[dfc, 2012-11-22 13:05:22]

Thank you for this proposal, some comments attached.

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go
File src/pkg/net/timeout_test.go (right):

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:21: ln, err := Listen("tcp", "127.0.0.1:0")
Why not just ListenTCP(...), then you don't need the type assertion ?

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:24: }
defer ln.Close()

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:27: if _, err := ln.Accept(); !err.(Error).Timeout()
{
This will panic if err is not a *net.Error. I guess that is the point, but the
t.Fatalf will not be reached if this isn't the expected type.

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:65: ln, err := Listen("tcp", "127.0.0.1:0")
ListenTCP(...)

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:70: c, err := Dial("tcp", ln.Addr().String())
DialTCP("tcp", nil, ln.Addr())

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:73: }
defer c.Close()

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:116: ln, err := Listen("tcp", "127.0.0.1:0")
Same as above

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:124: }
defer c.Close()

[dvyukov, 2012-11-22 13:15:59]

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go
File src/pkg/net/timeout_test.go (right):

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:21: ln, err := Listen("tcp", "127.0.0.1:0")
On 2012/11/22 13:05:22, dfc wrote:
> Why not just ListenTCP(...), then you don't need the type assertion ?

What is the easy way to create TCPAddr?

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:24: }
On 2012/11/22 13:05:22, dfc wrote:
> defer ln.Close()

It must be closed not at the end of function.

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:73: }
On 2012/11/22 13:05:22, dfc wrote:
> defer c.Close()

It is closed not at the end of function

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:124: }
On 2012/11/22 13:05:22, dfc wrote:
> defer c.Close()

It is closed before function end

[dfc, 2012-11-22 13:21:21]

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go
File src/pkg/net/timeout_test.go (right):

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:21: ln, err := Listen("tcp", "127.0.0.1:0")
> What is the easy way to create TCPAddr?

t := &net.TCPAddr{
    IP: net.IP{127,0,0,1},
    Port: 0,
}

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:24: }
On 2012/11/22 13:15:59, dvyukov wrote:
> On 2012/11/22 13:05:22, dfc wrote:
> > defer ln.Close()
> 
> It must be closed not at the end of function.

But it will leak if the test fails otherwise.

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:124: }
> It is closed before function end

Not if any t.Fatal fires before it

[dvyukov, 2012-11-23 07:02:58]

On 2012/11/22 13:21:21, dfc wrote:
> https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go
> File src/pkg/net/timeout_test.go (right):
> 
>
https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:21: ln, err := Listen("tcp", "127.0.0.1:0")
> > What is the easy way to create TCPAddr?
> 
> t := &net.TCPAddr{
>     IP: net.IP{127,0,0,1},
>     Port: 0,
> }
> 
>
https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:24: }
> On 2012/11/22 13:15:59, dvyukov wrote:
> > On 2012/11/22 13:05:22, dfc wrote:
> > > defer ln.Close()
> > 
> > It must be closed not at the end of function.
> 
> But it will leak if the test fails otherwise.
> 
>
https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:124: }
> > It is closed before function end
> 
> Not if any t.Fatal fires before it


All Done. PTAL.

[dfc, 2012-11-23 11:31:35]

Thank you. LGTM, all tests pass on linux/arm

[dvyukov, 2012-11-23 11:46:59]

Brad?


On Fri, Nov 23, 2012 at 3:31 PM, <dave@cheney.net> wrote:

> Thank you. LGTM, all tests pass on linux/arm
>
>
https://codereview.appspot.**com/6850070/<https://codereview.appspot.com/6850...
>

[bradfitz, 2012-11-24 05:00:15]

LGTM

https://codereview.appspot.com/6850070/diff/1004/src/pkg/net/timeout_test.go
File src/pkg/net/timeout_test.go (right):

https://codereview.appspot.com/6850070/diff/1004/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:21: ln, err := ListenTCP("tcp", &TCPAddr{IP: IP{127,
0, 0, 1}, Port: 0})
ln, err := newLocalListener()

?

https://codereview.appspot.com/6850070/diff/1004/src/pkg/net/timeout_test.go#...
src/pkg/net/timeout_test.go:69: ln, err := ListenTCP("tcp", &TCPAddr{IP: IP{127,
0, 0, 1}, Port: 0})
ln, err := newLocalListener()

?

[dvyukov, 2012-11-25 09:27:14]

On 2012/11/24 05:00:15, bradfitz wrote:
> LGTM
> 
> https://codereview.appspot.com/6850070/diff/1004/src/pkg/net/timeout_test.go
> File src/pkg/net/timeout_test.go (right):
> 
>
https://codereview.appspot.com/6850070/diff/1004/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:21: ln, err := ListenTCP("tcp", &TCPAddr{IP:
IP{127,
> 0, 0, 1}, Port: 0})
> ln, err := newLocalListener()
> 
> ?
> 
>
https://codereview.appspot.com/6850070/diff/1004/src/pkg/net/timeout_test.go#...
> src/pkg/net/timeout_test.go:69: ln, err := ListenTCP("tcp", &TCPAddr{IP:
IP{127,
> 0, 0, 1}, Port: 0})
> ln, err := newLocalListener()
> 
> ?


Done.
I've also made it use new isTimeout() function.

[dvyukov, 2012-11-25 09:27:51]

*** Submitted as http://code.google.com/p/go/source/detail?r=d351a7cc9ca9 ***

net: add unit tests for read/write deadlines
The tests verify that deadlines are "persistent",
read/write deadlines do not interfere, can be reset,
read deadline can be set with both SetDeadline()
and SetReadDeadline(), etc.

R=golang-dev, bradfitz, dave
CC=golang-dev
http://codereview.appspot.com/6850070