[golang-dev] code review 6850070: net: add unit tests for read/write deadlines (issue 6850070)

[golang-dev] code review 6850070: net: add unit tests for read/write deadlines (issue 6850070)

	Dvyukov
	Nov 18, 2012 at 2:42 pm

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com,

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

Description:
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.

Please review this at http://codereview.appspot.com/6850070/

Affected files:
M src/pkg/net/timeout_test.go

Search Discussions

11 responses
Oldest
Nested

Bradfitz

at Nov 20, 2012 at 2:05 am

⇧

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#newcode26
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#newcode49
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#newcode90
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#newcode100
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#newcode143
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#newcode157
src/pkg/net/timeout_test.go:157: t.Fatalf("Expected Write() to not
return, but it retrned with %v\n", err)
returned

https://codereview.appspot.com/6850070/

reply | permalink

Dvyukov

at Nov 22, 2012 at 9:22 am

⇧

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#newcode26

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#newcode49

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#newcode90

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#newcode100

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#newcode143

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#newcode157

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.

https://codereview.appspot.com/6850070/

reply | permalink

Dave

at Nov 22, 2012 at 1:11 pm

⇧

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#newcode21
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#newcode24
src/pkg/net/timeout_test.go:24: }
defer ln.Close()

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#newcode27
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#newcode65
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#newcode70
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#newcode73
src/pkg/net/timeout_test.go:73: }
defer c.Close()

https://codereview.appspot.com/6850070/diff/1003/src/pkg/net/timeout_test.go#newcode116
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#newcode124
src/pkg/net/timeout_test.go:124: }
defer c.Close()

https://codereview.appspot.com/6850070/

reply | permalink

Dvyukov

at Nov 22, 2012 at 1:21 pm

⇧

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#newcode21
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#newcode24
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#newcode73
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#newcode124
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

https://codereview.appspot.com/6850070/

reply | permalink

Dave

at Nov 22, 2012 at 1:21 pm

⇧

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#newcode24
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#newcode124
src/pkg/net/timeout_test.go:124: }

It is closed before function end

Not if any t.Fatal fires before it

https://codereview.appspot.com/6850070/

reply | permalink

Dvyukov

at Nov 23, 2012 at 7:02 am

⇧

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#newcode21

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#newcode24

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#newcode124

src/pkg/net/timeout_test.go:124: }

It is closed before function end

Not if any t.Fatal fires before it

All Done. PTAL.

https://codereview.appspot.com/6850070/

reply | permalink

Dave Thank you. LGTM, all tests pass on linux/arm https://codereview.appspot.com/6850070/

Dave at Nov 23, 2012 at 11:31 am ⇧

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

https://codereview.appspot.com/6850070/

reply | permalink
Dmitry Vyukov Brad?

Dmitry Vyukov at Nov 23, 2012 at 11:46 am ⇧

Brad?

On Fri, Nov 23, 2012 at 3:31 PM, wrote:

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

https://codereview.appspot.**com/6850070/<https://codereview.appspot.com/6850070/>

reply | permalink

Bradfitz

at Nov 24, 2012 at 5:00 am

⇧

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#newcode21
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#newcode69
src/pkg/net/timeout_test.go:69: ln, err := ListenTCP("tcp", &TCPAddr{IP:
IP{127, 0, 0, 1}, Port: 0})
ln, err := newLocalListener()

?

https://codereview.appspot.com/6850070/

reply | permalink

Dvyukov

at Nov 25, 2012 at 9:27 am

⇧

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#newcode21

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#newcode69

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.

https://codereview.appspot.com/6850070/

reply | permalink

Dvyukov

at Nov 25, 2012 at 9:27 am

⇧

*** 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

http://codereview.appspot.com/6850070/

reply | permalink

Related Discussions

Discussion Navigation

view	thread \| post

Discussion Overview

group	golang-dev @
categories	go
posted	Nov 18, '12 at 2:42p
active	Nov 25, '12 at 9:27a
posts	12
users	3
website	golang.org

responses	expanded	Hotkey: s
font	variable	Hotkey: f
user style	avatars	Hotkey: a

[golang-dev] code review 6850070: net: add unit tests for read/write deadlines (issue 6850070)

Search Discussions

Related Discussions

3 users in discussion