code review 6850110: net: move deadline logic into pollServer

net: move deadline logic into pollServer

Update issue 4434.

The proposal attempts to reduce the number of places where fd,{r,w}deadline is checked and updated in preparation for issue 4434. In doing so the deadline logic is simplified by letting the pollster return errTimeout from netFD.Wait{Read,Write} as part of the wakeup logic.

The behaviour of setting n = 0 has been restored to match rev 2a55e349097f, which was the previous change to fd_unix.go before CL 6851096.

[dfc, 2012-11-27 09:18:24]

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

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

[bradfitz, 2012-11-27 09:42:07]

So far looking nice, and simpler.

You going to do the rest in this same CL?

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

https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:40: // owned by client
in one of these rounds I'd like to see this whole section cleaned up and
documented. e.g. ownership, locking rules, what does rdeadline <0, 0, >0 mean
(and that they're unix nanos), etc.

[dfc, 2012-11-27 09:52:29]

I would like to do this as two separate CLs. I'd also like to get some feedback
from jsing, as this CL replaces his, and hopefully addresses his concerns.

Although the goal is to make no change, I would like to see it passing on the
builders before replacing the rdeadline,wdeadline fields with atomic accessors. 

That latter change will be pretty straight forward now there are less places
where we set and check the deadlines.

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

https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:40: // owned by client
On 2012/11/27 09:42:07, bradfitz wrote:
> in one of these rounds I'd like to see this whole section cleaned up and
> documented. e.g. ownership, locking rules, what does rdeadline <0, 0, >0 mean
> (and that they're unix nanos), etc.

Will do in a followup CL

[mikio, 2012-11-27 12:34:36]

It passes on openbsd, yay.

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

https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype !=
syscall.SOCK_DGRAM {
fd.sotype == syscall.SOCK_STREAM

https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:468: }
if n == 0 && err == nil && fd.sotype == syscall.SOCK_STREAM {
  err = io.EOF
} 

It's possible to transmit UDP payload size = 0 datagram.

https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:500: if err == nil && n == 0 {
if n == 0 && err == nil && fd.sotype == syscall.SOCK_STREAM {

It's possible to transmit UDP payload size = 0 datagram.

[dfc, 2012-11-27 12:48:58]

Thank you for your comments. I am encouraged to hear OpenBSD is passing. PTAL.

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

https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype !=
syscall.SOCK_DGRAM {
On 2012/11/27 12:34:36, mikio wrote:
> fd.sotype == syscall.SOCK_STREAM

Done.

https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:468: }
On 2012/11/27 12:34:36, mikio wrote:
> if n == 0 && err == nil && fd.sotype == syscall.SOCK_STREAM {
>   err = io.EOF
> } 
> 
> It's possible to transmit UDP payload size = 0 datagram.

Done.

https://codereview.appspot.com/6850110/diff/6003/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:500: if err == nil && n == 0 {
On 2012/11/27 12:34:36, mikio wrote:
> if n == 0 && err == nil && fd.sotype == syscall.SOCK_STREAM {
> 
> It's possible to transmit UDP payload size = 0 datagram.

Done, please note the TODO on line 493.

[rsc, 2012-11-27 15:26:58]

LGTM

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

https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype ==
syscall.SOCK_STREAM {
Please change this back to != SOCK_DGRAM.
This is mishandling SOCK_SEQPACKET.

https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:469: if n == 0 && err == nil && fd.sotype ==
syscall.SOCK_STREAM {
Same.

https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:503: if n == 0 && err == nil && fd.sotype ==
syscall.SOCK_STREAM {
Same.

[jsing, 2012-11-27 16:01:55]

LGTM.

This is the less intrusive change, although more expensive in terms of
consecutive calls to read/write after the deadline has passed - I'm not
convinced this matters though. I still think there may be benefit in factoring
out the "check deadline" code, which is currently sprinkled in multiple places.

FTR this makes the tests pass again on a machine similar to the OpenBSD/amd64
builder and fixes the failure of the TestReadDeadlineConsecutive test provided
in https://codereview.appspot.com/6842101/.

[mikio, 2012-11-27 21:23:18]

LGTM

On Wed, Nov 28, 2012 at 12:26 AM,  <rsc@golang.org> wrote:

>
https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
> src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype ==
> syscall.SOCK_STREAM {
> Please change this back to != SOCK_DGRAM.
> This is mishandling SOCK_SEQPACKET.

Ah, I forgot it. I'm fine with except SOCK_DGRAM and SOCK_RAW.


On Tue, Nov 27, 2012 at 9:48 PM,  <dave@cheney.net> wrote:

> Done, please note the TODO on line 493.

n, oobn = 0, 0 at L494? I guess it works for ReadMsgUnix on
stream-type UnixConn.
(not tested yet, need more test cases...)

[dfc, 2012-11-27 21:35:16]

PTAL. I'm leaving this code to stress test for a few hours, then will submit if
nothing bad falls out.

@jsing - i believe that in the case where Read is called after the deadline has
passed the cost is about the same as the original code in rev 2a55e349097f.
Previously a call to netFD.Read would have executed syscall.Read
unconditionally, then gone into WaitRead, waited for the lock, gone into AddFD,
and called time.Now before detecting the deadline had passed. Now the check of
the deadline is done at the top of the loop. If the deadline passes after the
check at the top, but before the non blocking syscall.Read, then the original
logic applies.

On my machine time.Now() is ~ 26ns, but may be higher on the BSDs that don't
have a fast syscall mechanism. 

@ mikioh - yes, I intend to write test cases for the lesser known Read{Msg,From}
variations to resolve that TODO.

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

https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype ==
syscall.SOCK_STREAM {
On 2012/11/27 15:26:58, rsc wrote:
> Please change this back to != SOCK_DGRAM.
> This is mishandling SOCK_SEQPACKET.

Done.

https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:469: if n == 0 && err == nil && fd.sotype ==
syscall.SOCK_STREAM {
On 2012/11/27 15:26:58, rsc wrote:
> Same.

Done.

https://codereview.appspot.com/6850110/diff/11002/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:503: if n == 0 && err == nil && fd.sotype ==
syscall.SOCK_STREAM {
On 2012/11/27 15:26:58, rsc wrote:
> Same.

Done.

[mikio, 2012-11-27 21:42:59]

nits

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

https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype !=
syscall.SOCK_DGRAM {
&& fd.sotype != syscall.SOCK_DGRAM && fd.sotype != syscall.SOCK_RAW

https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:469: if n == 0 && err == nil && fd.sotype !=
syscall.SOCK_DGRAM {
ditto

https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:503: if n == 0 && err == nil && fd.sotype !=
syscall.SOCK_DGRAM {
ditto

[dfc, 2012-11-27 23:48:43]

Hopefully this is the final tweak. As the logic of that check was getting large,
I have moved it into a helper. PTAL.

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

https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype !=
syscall.SOCK_DGRAM {
On 2012/11/27 21:42:59, mikio wrote:
> && fd.sotype != syscall.SOCK_DGRAM && fd.sotype != syscall.SOCK_RAW

Done.

https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:469: if n == 0 && err == nil && fd.sotype !=
syscall.SOCK_DGRAM {
On 2012/11/27 21:42:59, mikio wrote:
> ditto

Done.

https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:503: if n == 0 && err == nil && fd.sotype !=
syscall.SOCK_DGRAM {
On 2012/11/27 21:42:59, mikio wrote:
> ditto

Done.

[mikio, 2012-11-28 00:06:55]

On Wed, Nov 28, 2012 at 8:48 AM,  <dave@cheney.net> wrote:

> Hopefully this is the final tweak. As the logic of that check was
> getting large, I have moved it into a helper. PTAL.

Thanks.

[bradfitz, 2012-11-28 00:07:53]

LGTM

I assume you'll do RecvFrom todo and tests later?
 On Nov 27, 2012 3:48 PM, <dave@cheney.net> wrote:

> Hopefully this is the final tweak. As the logic of that check was
> getting large, I have moved it into a helper. PTAL.
>
>
> https://codereview.appspot.**com/6850110/diff/2005/src/pkg/**
>
net/fd_unix.go<https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go>
> File src/pkg/net/fd_unix.go (right):
>
> https://codereview.appspot.**com/6850110/diff/2005/src/pkg/**
>
net/fd_unix.go#newcode435<https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go#newcode435>
> src/pkg/net/fd_unix.go:435: if n == 0 && err == nil && fd.sotype !=
> syscall.SOCK_DGRAM {
> On 2012/11/27 21:42:59, mikio wrote:
>
>> && fd.sotype != syscall.SOCK_DGRAM && fd.sotype != syscall.SOCK_RAW
>>
>
> Done.
>
> https://codereview.appspot.**com/6850110/diff/2005/src/pkg/**
>
net/fd_unix.go#newcode469<https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go#newcode469>
> src/pkg/net/fd_unix.go:469: if n == 0 && err == nil && fd.sotype !=
> syscall.SOCK_DGRAM {
> On 2012/11/27 21:42:59, mikio wrote:
>
>> ditto
>>
>
> Done.
>
> https://codereview.appspot.**com/6850110/diff/2005/src/pkg/**
>
net/fd_unix.go#newcode503<https://codereview.appspot.com/6850110/diff/2005/src/pkg/net/fd_unix.go#newcode503>
> src/pkg/net/fd_unix.go:503: if n == 0 && err == nil && fd.sotype !=
> syscall.SOCK_DGRAM {
> On 2012/11/27 21:42:59, mikio wrote:
>
>> ditto
>>
>
> Done.
>
>
https://codereview.appspot.**com/6850110/<https://codereview.appspot.com/6850...
>

[dfc, 2012-11-28 00:29:48]

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

net: move deadline logic into pollServer

Update issue 4434.

The proposal attempts to reduce the number of places where fd,{r,w}deadline is
checked and updated in preparation for issue 4434. In doing so the deadline
logic is simplified by letting the pollster return errTimeout from
netFD.Wait{Read,Write} as part of the wakeup logic.

The behaviour of setting n = 0 has been restored to match rev 2a55e349097f,
which was the previous change to fd_unix.go before CL 6851096.

R=jsing, bradfitz, mikioh.mikioh, rsc
CC=fullung, golang-dev
http://codereview.appspot.com/6850110

[bradfitz, 2012-11-28 01:22:58]

Okay, time to make deadline updates atomic!

On Tue, Nov 27, 2012 at 4:29 PM, <dave@cheney.net> wrote:

> *** Submitted as
>
http://code.google.com/p/go/**source/detail?r=1289e67dec66<http://code.google...
>
>
> net: move deadline logic into pollServer
>
> Update issue 4434.
>
> The proposal attempts to reduce the number of places where
> fd,{r,w}deadline is checked and updated in preparation for issue 4434.
> In doing so the deadline logic is simplified by letting the pollster
> return errTimeout from netFD.Wait{Read,Write} as part of the wakeup
> logic.
>
> The behaviour of setting n = 0 has been restored to match rev
> 2a55e349097f, which was the previous change to fd_unix.go before CL
> 6851096.
>
> R=jsing, bradfitz, mikioh.mikioh, rsc
> CC=fullung, golang-dev
>
http://codereview.appspot.com/**6850110<http://codereview.appspot.com/6850110>
>
>
>
http://codereview.appspot.com/**6850110/<http://codereview.appspot.com/6850110/>
>