code review 7436048: net: fix accept/connect deadline handling

net: fix accept/connect deadline handling
Ensure that accept/connect respect deadline,
even if the operation can be executed w/o blocking.
Note this changes external behavior, but it makes
it consistent with read/write.
Factor out deadline check into pollServer.PrepareRead/Write,
in preparation for edge triggered pollServer.
Ensure that pollServer.WaitRead/Write are not called concurrently
by adding rio/wio locks around connect/accept.

[dvyukov, 2013-03-01 08:49:59]

Hello golang-dev@googlegroups.com,

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

[mikio, 2013-03-01 11:13:09]

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

https://codereview.appspot.com/7436048/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:355: return err
&OpError{"connect", fd.net, fd.raddr, err}
or will fix later in issue 4856?

https://codereview.appspot.com/7436048/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:452: return 0, &OpError{"read", fd.net, fd.raddr, err}
fd.laddr?

https://codereview.appspot.com/7436048/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:468: err = &OpError{"read", fd.net, fd.raddr, err}
fd.laddr?

[dvyukov, 2013-03-01 11:55:44]

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

https://codereview.appspot.com/7436048/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:355: return err
On 2013/03/01 11:13:09, mikio wrote:
> &OpError{"connect", http://fd.net, fd.raddr, err}
> or will fix later in issue 4856?

I am just making it consistent with the code below. It calls WaitWrite() and
returns the err directly. It may be wrapper somewhere above, I don't know.

https://codereview.appspot.com/7436048/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:452: return 0, &OpError{"read", fd.net, fd.raddr, err}
On 2013/03/01 11:13:09, mikio wrote:
> fd.laddr?

Done.

https://codereview.appspot.com/7436048/diff/8001/src/pkg/net/fd_unix.go#newco...
src/pkg/net/fd_unix.go:468: err = &OpError{"read", fd.net, fd.raddr, err}
On 2013/03/01 11:13:09, mikio wrote:
> fd.laddr?

Done.
It was another bug, right?

[dvyukov, 2013-03-01 11:56:16]

PTAL

Is it the right thing to check deadline before connect/accept?

[dvyukov, 2013-03-01 11:58:04]

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

https://codereview.appspot.com/7436048/diff/13001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:634: defer fd.rio.Unlock()
Is it fine? It somewhat limits parallelism. As far as I know, must uses in std
lib call accept() from a single goroutine, so it's not a problem, but there can
be other use cases.

[mikio, 2013-03-01 12:09:54]

> Done.
> It was another bug, right?

light. sorry, right.

[mikio, 2013-03-01 12:54:47]

On Fri, Mar 1, 2013 at 8:58 PM,  <dvyukov@google.com> wrote:

> Is it fine?

I think it's okay.

> It somewhat limits parallelism. As far as I know, must uses
> in std lib call accept() from a single goroutine, so it's not a problem,
> but there can be other use cases.

like this?

func worker(fd, ...) {
  // monitor active sessions, control backlogs
  nfd, err := fd.Accept()
  go worker(fd, ...)
  n, err := nfd.Read()
  :
}

[dvyukov, 2013-03-01 14:34:06]

On Fri, Mar 1, 2013 at 2:54 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:

> On Fri, Mar 1, 2013 at 8:58 PM,  <dvyukov@google.com> wrote:
>
> > Is it fine?
>
> I think it's okay.
>
> > It somewhat limits parallelism. As far as I know, must uses
> > in std lib call accept() from a single goroutine, so it's not a problem,
> > but there can be other use cases.
>
> like this?
>
> func worker(fd, ...) {
>   // monitor active sessions, control backlogs
>   nfd, err := fd.Accept()
>   go worker(fd, ...)
>   n, err := nfd.Read()
>   :
> }
>

This should be find. I mean when several goroutines concurrently call
accept().

[dvyukov, 2013-03-02 06:19:56]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[dvyukov, 2013-03-02 08:16:33]

On 2013/03/02 06:19:56, dvyukov wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:mikioh.mikioh@gmail.com (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Brad, can you take a look at it?

[mikio, 2013-03-02 08:30:02]

LGTM

[bradfitz, 2013-03-04 17:31:06]

LGTM but others may have opinions

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

https://codereview.appspot.com/7436048/diff/19001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:237: func (s *pollServer) PrepareRead(fd *netFD) error {
"Prepare" makes it sound like more is happening. It's more like "ShouldRead" or
"CanRead" or "ReadOkay".

https://codereview.appspot.com/7436048/diff/19001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:350: if err := fd.pollServer.PrepareWrite(fd); err != nil
{
can/should this be done before the wio.Lock?

[dvyukov, 2013-03-04 20:49:40]

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

https://codereview.appspot.com/7436048/diff/19001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:237: func (s *pollServer) PrepareRead(fd *netFD) error {
On 2013/03/04 17:31:06, bradfitz wrote:
> "Prepare" makes it sound like more is happening. It's more like "ShouldRead"
or
> "CanRead" or "ReadOkay".

Edge-triggered poll server will reset any pending notifications in this function
if any. So it will be more than "ShouldRead".

https://codereview.appspot.com/7436048/diff/19001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:350: if err := fd.pollServer.PrepareWrite(fd); err != nil
{
On 2013/03/04 17:31:06, bradfitz wrote:
> can/should this be done before the wio.Lock?

I think it will be able to lead to deadlocks for edge triggered poll server if
done outside of lock (in case connect is called concurrently from several
goroutines).

[dvyukov, 2013-03-05 08:17:37]

On 2013/03/04 17:31:06, bradfitz wrote:
> LGTM but others may have opinions

Waiting for others for 1 more day

[iant, 2013-03-05 14:25:56]

LGTM

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

https://codereview.appspot.com/7436048/diff/19001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:448: return 0, &OpError{"read", fd.net, fd.laddr, err}
Why do you return an OpError here but not for the similar case in connect? 
Seems like connect could return an OpError as well.

https://codereview.appspot.com/7436048/diff/19001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:464: err = &OpError{"read", fd.net, fd.laddr, err}
Why change to use laddr here?  It seems more useful to use raddr for a read from
a connected socket.

[dvyukov, 2013-03-05 15:17:52]

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

https://codereview.appspot.com/7436048/diff/19001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:448: return 0, &OpError{"read", fd.net, fd.laddr, err}
On 2013/03/05 14:25:57, iant wrote:
> Why do you return an OpError here but not for the similar case in connect?
> Seems like connect could return an OpError as well.

Here I return OpError, because OpError is currently returned here.

In connect I return raw error, because connect currently returns raw error (e.g.
directly returns err from WaitWrite()).

I just tried to make conservative changes in this aspect. I've filed
https://code.google.com/p/go/issues/detail?id=4856 to make the error values
consistent.

https://codereview.appspot.com/7436048/diff/19001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:464: err = &OpError{"read", fd.net, fd.laddr, err}
On 2013/03/05 14:25:57, iant wrote:
> Why change to use laddr here?  It seems more useful to use raddr for a read
from
> a connected socket.

Why do we return laddr from ReadFrom/ReadMsg? ReadMsg can be called on connected
socket as well. And user passes explicit sa to ReadFrom/ReadMsg, should we print
them?

I am changing this back to raddr. Does everybody agree? PTAL.

[iant, 2013-03-05 17:13:05]

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

https://codereview.appspot.com/7436048/diff/19001/src/pkg/net/fd_unix.go#newc...
src/pkg/net/fd_unix.go:464: err = &OpError{"read", fd.net, fd.laddr, err}
On 2013/03/05 15:17:52, dvyukov wrote:
> On 2013/03/05 14:25:57, iant wrote:
> > Why change to use laddr here?  It seems more useful to use raddr for a read
> from
> > a connected socket.
> 
> Why do we return laddr from ReadFrom/ReadMsg? ReadMsg can be called on
connected
> socket as well. And user passes explicit sa to ReadFrom/ReadMsg, should we
print
> them?
> 
> I am changing this back to raddr. Does everybody agree? PTAL.

Readmsg can be called on a connected socket.  But it often is not.  Returning
laddr there seems appropriate as it most directly indicates the failing socket. 
But Read is always called on a connected socket, and the local address is almost
always irrelevant.  The remote address is the one of interest.

[mikio, 2013-03-05 20:23:59]

On Wed, Mar 6, 2013 at 2:13 AM,  <iant@golang.org> wrote:

> Readmsg can be called on a connected socket.  But it often is not.
> Returning laddr there seems appropriate as it most directly indicates
> the failing socket.  But Read is always called on a connected socket,
> and the local address is almost always irrelevant.  The remote address
> is the one of interest.

We allow both IPConn and UnixConn to use Read method.
UnixConn supports unnamed sockets and I'm not expecting
IPConn to return meaningful its remote address.

[dvyukov, 2013-03-05 21:05:35]

On Tue, Mar 5, 2013 at 10:23 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Wed, Mar 6, 2013 at 2:13 AM,  <iant@golang.org> wrote:
>
>> Readmsg can be called on a connected socket.  But it often is not.
>> Returning laddr there seems appropriate as it most directly indicates
>> the failing socket.  But Read is always called on a connected socket,
>> and the local address is almost always irrelevant.  The remote address
>> is the one of interest.


Perhaps we need to file a separate issue.  I don't want to change
anything here in this CL, since it's so controversial.


> We allow both IPConn and UnixConn to use Read method.
> UnixConn supports unnamed sockets and I'm not expecting
> IPConn to return meaningful its remote address.

[iant, 2013-03-05 21:35:40]

On Tue, Mar 5, 2013 at 1:05 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Mar 5, 2013 at 10:23 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
>> On Wed, Mar 6, 2013 at 2:13 AM,  <iant@golang.org> wrote:
>>
>>> Readmsg can be called on a connected socket.  But it often is not.
>>> Returning laddr there seems appropriate as it most directly indicates
>>> the failing socket.  But Read is always called on a connected socket,
>>> and the local address is almost always irrelevant.  The remote address
>>> is the one of interest.
>
>
> Perhaps we need to file a separate issue.  I don't want to change
> anything here in this CL, since it's so controversial.

But, but, I only started commenting because you changed raddr to
laddr.  If you omit that change from this CL I'll be fine with this.

Ian

[dvyukov, 2013-03-05 21:41:37]

I've already changed it back.


On Tue, Mar 5, 2013 at 11:35 PM, Ian Lance Taylor <iant@golang.org> wrote:
> On Tue, Mar 5, 2013 at 1:05 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Tue, Mar 5, 2013 at 10:23 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
>>> On Wed, Mar 6, 2013 at 2:13 AM,  <iant@golang.org> wrote:
>>>
>>>> Readmsg can be called on a connected socket.  But it often is not.
>>>> Returning laddr there seems appropriate as it most directly indicates
>>>> the failing socket.  But Read is always called on a connected socket,
>>>> and the local address is almost always irrelevant.  The remote address
>>>> is the one of interest.
>>
>>
>> Perhaps we need to file a separate issue.  I don't want to change
>> anything here in this CL, since it's so controversial.
>
> But, but, I only started commenting because you changed raddr to
> laddr.  If you omit that change from this CL I'll be fine with this.
>
> Ian

[mikio, 2013-03-05 22:56:25]

LGTM

> I've already changed it back.

>>> Perhaps we need to file a separate issue.  I don't want to change
>>> anything here in this CL, since it's so controversial.

SGTM

[dvyukov, 2013-03-07 13:04:04]

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

net: fix accept/connect deadline handling
Ensure that accept/connect respect deadline,
even if the operation can be executed w/o blocking.
Note this changes external behavior, but it makes
it consistent with read/write.
Factor out deadline check into pollServer.PrepareRead/Write,
in preparation for edge triggered pollServer.
Ensure that pollServer.WaitRead/Write are not called concurrently
by adding rio/wio locks around connect/accept.

R=golang-dev, mikioh.mikioh, bradfitz, iant
CC=golang-dev
https://codereview.appspot.com/7436048