code review 7795050: net: avoid use of listener socket options on active ope...

net: avoid use of listener socket options on active open sockets

This CL ensures we use the correct socket options for
passive and active open sockets.

For the passive open sockets created by Listen functions,
additional SO_REUSEADDR, SO_REUSEPORT options are required
for the quick service restart and/or multicasting.

For the active open sockets created by Dial functions, no
additional options are required.

[mikio, 2013-03-21 10:38:18]

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

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

[dave_cheney.net, 2013-03-21 11:12:26]

On 2013/03/21 10:38:18, mikio wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

Hi Mikio,

This change looks pretty self contained, but I'm having trouble understanding
what this is fixing. Is there an issue in the issue tracker or a test that could
help me understand how the fix works ?

Cheers

Dave

[dave_cheney.net, 2013-03-21 12:03:19]

On 2013/03/21 11:12:26, dfc wrote:
> On 2013/03/21 10:38:18, mikio wrote:
> > Hello mailto:golang-dev@googlegroups.com (cc:
> mailto:golang-dev@googlegroups.com),
> > 
> > I'd like you to review this change to
> > https://code.google.com/p/go
> 
> Hi Mikio,
> 
> This change looks pretty self contained, but I'm having trouble understanding
> what this is fixing. Is there an issue in the issue tracker or a test that
could
> help me understand how the fix works ?
> 
> Cheers
> 
> Dave

ok, I think I understand now, would this description accurate ? 

Avoid setting SO_REUSEADDR on laddr during Dial{,UDP,TCP}.

Or did I misunderstand ?

[bradfitz, 2013-03-21 14:39:31]

Yes, more context would be useful.

If you need a place to dump background information (error output, pcap
files, etc), feel free to open a bug just so you can reference it.


On Thu, Mar 21, 2013 at 4:12 AM, <dave@cheney.net> wrote:

> On 2013/03/21 10:38:18, mikio wrote:
>
>> Hello mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>(cc:
>>
> mailto:golang-dev@**googlegroups.com <golang-dev@googlegroups.com>),
>
>
>  I'd like you to review this change to
>> https://code.google.com/p/go
>>
>
> Hi Mikio,
>
> This change looks pretty self contained, but I'm having trouble
> understanding what this is fixing. Is there an issue in the issue
> tracker or a test that could help me understand how the fix works ?
>
> Cheers
>
> Dave
>
>
https://codereview.appspot.**com/7795050/<https://codereview.appspot.com/7795...
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[rsc, 2013-03-21 20:10:47]

It would really help if your CLs did not delete or insert unrelated blank lines.

[dave_cheney.net, 2013-03-22 04:00:14]

Mikio,

I think I understand what this change does now. If you can please
update the description with a better explanation and revert the
whitespace changes then I think it's good to submit.

re: whitespace, I think its best to hold off on that sort of change
til 1.1 is out.

On Fri, Mar 22, 2013 at 7:10 AM,  <rsc@golang.org> wrote:
> It would really help if your CLs did not delete or insert unrelated
> blank lines.
>
>
> https://codereview.appspot.com/7795050/

[dave_cheney.net, 2013-03-22 12:32:52]

Does this change address this occasional test failure ?

--- FAIL: TestDualStackTCPListener-95 (0.00 seconds)
        unicast_posix_test.go:407: Second Listen("tcp", "127.0.0.1:55478")
returns listen tcp 127.0.0.1:55478: address already in use, expected <nil>
FAIL

[mikio, 2013-03-25 04:20:07]

On Thu, Mar 21, 2013 at 8:12 PM,  <dave@cheney.net> wrote:

> This change looks pretty self contained, but I'm having trouble
> understanding what this is fixing. Is there an issue in the issue
> tracker or a test that could help me understand how the fix works ?

Ah, yup, it just fixes the stuff looks incorrect (or not necessary) and
not sure what's the issue.

Will look later, perhaps it doesn't matter in real use cases because
a socket w/ SO_REUSEADDR just does local address lookup w/
the given one but the socket consists of 5-tuple, I mean, there's
almost no chance to happen accidental cross talks even if the option
is enabled in active open sockets, hm.

[mikio, 2013-03-25 04:22:22]

Sorry for the late response and thanks for your detective work.

On Fri, Mar 22, 2013 at 9:32 PM,  <dave@cheney.net> wrote:

> Does this change address this occasional test failure ?
>
> --- FAIL: TestDualStackTCPListener-95 (0.00 seconds)
>         unicast_posix_test.go:407: Second Listen("tcp",
> "127.0.0.1:55478") returns listen tcp 127.0.0.1:55478: address already
> in use, expected <nil>
> FAIL

Not sure, but probably no, unfortunately.

[mikio, 2013-03-29 15:09:55]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org,
rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2013-03-30 00:33:48]

LGTM. Thank you, with minor comment comments

Can you please revise the description again, perhaps something like this:

This CL ensures we use the correct socket options for passive and
active open sockets.

For the passive open sockets created by Listen functions,
additional SO_REUSEADDR, SO_REUSEPORT options are required for
the quick service restart.

For the active open sockets created by Dial functions, no additional 
options are required.

https://codereview.appspot.com/7795050/diff/22001/src/pkg/net/sock_posix.go
File src/pkg/net/sock_posix.go (right):

https://codereview.appspot.com/7795050/diff/22001/src/pkg/net/sock_posix.go#n...
src/pkg/net/sock_posix.go:27: 
This is really subtle stuff due to the way net.socket is called for both
listening and connecting sockets. Can you please add a comment here, something
to the effect of:

// this socket is used by a listener, so set the usual
// socket options (SO_REUSEDADDR, etc (fill in the blanks))

https://codereview.appspot.com/7795050/diff/22001/src/pkg/net/sock_posix.go#n...
src/pkg/net/sock_posix.go:41: 
// otherwise the socket is being used to connect, so skip 
// SO_REUSEADDR and friends.

[mikio, 2013-03-30 06:21:32]

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

net: avoid use of listener socket options on active open sockets

This CL ensures we use the correct socket options for
passive and active open sockets.

For the passive open sockets created by Listen functions,
additional SO_REUSEADDR, SO_REUSEPORT options are required
for the quick service restart and/or multicasting.

For the active open sockets created by Dial functions, no
additional options are required.

R=golang-dev, dave, bradfitz, rsc
CC=golang-dev
https://codereview.appspot.com/7795050