code review 5562048: net: ListenMulticastUDP to listen concurrently across m...

net: ListenMulticastUDP to listen concurrently across multiple listeners

This CL introduces new function ListenMulticastUDP to fix
multicast UDP listening across multiple listeners issue,
to replace old multicast methods JoinGroup and LeaveGroup
on UDPConn.

This CL also enables multicast testing by default.

Fixes issue 2730.

[mikio, 2012-01-25 13:39:20]

Hello rsc@golang.org, paul.a.lalonde@gmail.com, fullung@gmail.com,
devon.odell@gmail.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-01-25 22:45:26]

based on our earlier email discussion,
i believe that this will be the end of the
multicast CLs before Go 1.
can you please confirm that this is the case?

also, following up on that discussion, now
that we have this new ListenMulticastUDP,
should we delete the JoinGroup and LeaveGroup
methods?

russ

[albert.strasheim, 2012-01-26 04:20:00]

LGTM. I'll try to get it tested in our multicast app today.

Re: Russ's question. I think people might want to be able to join a socket to
multiple groups (ListenMulticastUDP only can only join one) to simplify certain
kinds of multicast processing code, but maybe JoinGroup and LeaveGroup should go
away until the more general IP options API is ready.

It might be worth it to ask golang-nuts if there are any other users of the
current multicast API out there?

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_bsd.go
File src/pkg/net/sockopt_bsd.go (right):

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_bsd.go#ne...
src/pkg/net/sockopt_bsd.go:38: func setDefaultMulticastSockopts(s int) {
Related to my question about errors and Close from the other day: I don't know
why setsockopt would fail, but I know that it could fail. Do we want to ignore
those errors in functions like this one, or should errors be returned from
syscall.Setsockopt* and then returned from here so that the caller can again
check for and return them?

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_linux.go
File src/pkg/net/sockopt_linux.go (right):

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_linux.go#...
src/pkg/net/sockopt_linux.go:34: syscall.SetsockoptInt(s, syscall.SOL_SOCKET,
syscall.SO_REUSEADDR, 1)
Same thing here.

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/udpsock_posix.go
File src/pkg/net/udpsock_posix.go (right):

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/udpsock_posix.go#...
src/pkg/net/udpsock_posix.go:264: setIPv4MulticastInterface(c.fd, ifi)
Whoohoo!

[mikio, 2012-01-26 13:19:37]

On Thu, Jan 26, 2012 at 7:45 AM, Russ Cox <rsc@golang.org> wrote:

> based on our earlier email discussion,
> i believe that this will be the end of the
> multicast CLs before Go 1.
> can you please confirm that this is the case?

Certainly.

> also, following up on that discussion, now
> that we have this new ListenMulticastUDP,
> should we delete the JoinGroup and LeaveGroup
> methods?

I've been thinking on that and I think it's a good time
to do that.

-- Mikio

[dho, 2012-01-26 15:09:09]

2012/1/25  <fullung@gmail.com>:
> LGTM. I'll try to get it tested in our multicast app today.
>
> Re: Russ's question. I think people might want to be able to join a
> socket to multiple groups (ListenMulticastUDP only can only join one) to
> simplify certain kinds of multicast processing code, but maybe JoinGroup
> and LeaveGroup should go away until the more general IP options API is
> ready.
>
> It might be worth it to ask golang-nuts if there are any other users of
> the current multicast API out there?

I think perhaps Paul will have some input on this.

> http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_bsd.go
> File src/pkg/net/sockopt_bsd.go (right):
>
>
http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_bsd.go#ne...
> src/pkg/net/sockopt_bsd.go:38: func setDefaultMulticastSockopts(s int) {
> Related to my question about errors and Close from the other day: I
> don't know why setsockopt would fail, but I know that it could fail. Do
> we want to ignore those errors in functions like this one, or should
> errors be returned from syscall.Setsockopt* and then returned from here
> so that the caller can again check for and return them?

I think errors should be returned. If SO_REUSEADDR fails when you're
trying to use it in a multicast context, it means that something else
is listening on that socket and it is only available for unicast
messages. People should be able to exit from this condition
gracefully.

> http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_linux.go
> File src/pkg/net/sockopt_linux.go (right):
>
>
http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_linux.go#...
> src/pkg/net/sockopt_linux.go:34: syscall.SetsockoptInt(s,
> syscall.SOL_SOCKET, syscall.SO_REUSEADDR, 1)
> Same thing here.

Idem to above then ;)

> http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/udpsock_posix.go
> File src/pkg/net/udpsock_posix.go (right):
>
>
http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/udpsock_posix.go#...
> src/pkg/net/udpsock_posix.go:264: setIPv4MulticastInterface(c.fd, ifi)
> Whoohoo!
>
> http://codereview.appspot.com/5562048/

--dho

[rsc, 2012-01-26 15:11:48]

Great.  Can you please remove the Group methods in this CL?

[mikio, 2012-01-26 16:24:47]

Will do

-- Mikio


On Jan 27, 2012, at 12:11 AM, Russ Cox <rsc@golang.org> wrote:

> Great.  Can you please remove the Group methods in this CL?

[mikio, 2012-01-28 14:28:40]

I think all comments are addressed.
- add ListenMulticastUDP
- remove both JoinGroup, LeaveGroup
- add error handling for sockopts

PTAL.

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_bsd.go
File src/pkg/net/sockopt_bsd.go (right):

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_bsd.go#ne...
src/pkg/net/sockopt_bsd.go:38: func setDefaultMulticastSockopts(s int) {
On 2012/01/26 04:20:01, albert.strasheim wrote:
> Related to my question about errors and Close from the other day: I don't know
> why setsockopt would fail, but I know that it could fail. Do we want to ignore
> those errors in functions like this one, or should errors be returned from
> syscall.Setsockopt* and then returned from here so that the caller can again
> check for and return them?

Done.

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_linux.go
File src/pkg/net/sockopt_linux.go (right):

http://codereview.appspot.com/5562048/diff/7015/src/pkg/net/sockopt_linux.go#...
src/pkg/net/sockopt_linux.go:34: syscall.SetsockoptInt(s, syscall.SOL_SOCKET,
syscall.SO_REUSEADDR, 1)
On 2012/01/26 04:20:01, albert.strasheim wrote:
> Same thing here.

Done.

[mikio, 2012-01-28 14:29:37]

Hello rsc@golang.org, paul.a.lalonde@gmail.com, fullung@gmail.com,
devon.odell@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[mikio, 2012-01-28 18:57:44]

Hello rsc@golang.org, paul.a.lalonde@gmail.com, fullung@gmail.com,
devon.odell@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dho, 2012-01-28 20:22:03]

Lgtm
On Jan 28, 2012 1:57 PM, <mikioh.mikioh@gmail.com> wrote:

> Hello rsc@golang.org, paul.a.lalonde@gmail.com, fullung@gmail.com,
> devon.odell@gmail.com (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5562048/<http://codereview.appspot.com/5562048/>
>

[rsc, 2012-01-29 17:48:28]

LGTM

http://codereview.appspot.com/5562048/diff/15054/doc/go1.tmpl
File doc/go1.tmpl (right):

http://codereview.appspot.com/5562048/diff/15054/doc/go1.tmpl#newcode1116
doc/go1.tmpl:1116: There are also new methods
s/methods/functions/

http://codereview.appspot.com/5562048/diff/15054/doc/go1.tmpl#newcode1121
doc/go1.tmpl:1121: The <code>net.ListenMulticastUDP</code> also replaces the old
s/also/function/

[mikio, 2012-01-31 16:53:32]

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

net: ListenMulticastUDP to listen concurrently across multiple listeners

This CL introduces new function ListenMulticastUDP to fix
multicast UDP listening across multiple listeners issue,
to replace old multicast methods JoinGroup and LeaveGroup
on UDPConn.

This CL also enables multicast testing by default.

Fixes issue 2730.

R=rsc, paul.a.lalonde, fullung, devon.odell
CC=golang-dev
http://codereview.appspot.com/5562048