code review 13457058: net: fix Close of autobind unix listener

net: fix Close of autobind unix listener

Fixes issue 6455.

[albert.strasheim, 2013-09-18 14:36:19]

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

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

[mikio, 2013-09-18 22:44:11]

could you please file an issue and update the cl descr. will take a look
tonight.

-- Mikio

On Sep 18, 2013, at 11:36 PM, fullung@gmail.com wrote:

> Reviewers: mikio,
> 
> Message:
> Hello mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go
> 
> 
> Description:
> net: fix Close of autobind unix listener
> 
> Please review this at https://codereview.appspot.com/13457058/
> 
> Affected files (+14, -2 lines):
>  M src/pkg/net/unix_test.go
>  M src/pkg/net/unixsock_posix.go
> 
> 
> Index: src/pkg/net/unix_test.go
> ===================================================================
> --- a/src/pkg/net/unix_test.go
> +++ b/src/pkg/net/unix_test.go
> @@ -107,7 +107,7 @@
>    }
> }
> 
> -func TestUnixAutobind(t *testing.T) {
> +func TestUnixgramAutobind(t *testing.T) {
>    if runtime.GOOS != "linux" {
>        t.Skip("skipping: autobind is linux only")
>    }
> @@ -139,6 +139,18 @@
>    }
> }
> 
> +func TestUnixAutobindClose(t *testing.T) {
> +    if runtime.GOOS != "linux" {
> +        t.Skip("skipping: autobind is linux only")
> +    }
> +    laddr := &UnixAddr{Name: "", Net: "unix"}
> +    ln, err := ListenUnix("unix", laddr)
> +    if err != nil {
> +        t.Fatalf("ListenUnix failed: %v", err)
> +    }
> +    ln.Close()
> +}
> +
> func TestUnixConnLocalAndRemoteNames(t *testing.T) {
>    for _, laddr := range []string{"", testUnixAddr()} {
>        laddr := laddr
> Index: src/pkg/net/unixsock_posix.go
> ===================================================================
> --- a/src/pkg/net/unixsock_posix.go
> +++ b/src/pkg/net/unixsock_posix.go
> @@ -271,7 +271,7 @@
>    if err != nil {
>        return nil, &OpError{Op: "listen", Net: net, Addr: laddr, Err: err}
>    }
> -    return &UnixListener{fd, laddr.Name}, nil
> +    return &UnixListener{fd, fd.laddr.String()}, nil
> }
> 
> // AcceptUnix accepts the next incoming call and returns the new
> 
> 

[mikio, 2013-09-19 19:58:27]

https://codereview.appspot.com/13457058/diff/11001/src/pkg/net/unixsock_posix.go
File src/pkg/net/unixsock_posix.go (right):

https://codereview.appspot.com/13457058/diff/11001/src/pkg/net/unixsock_posix...
src/pkg/net/unixsock_posix.go:274: return &UnixListener{fd, fd.laddr.String()},
nil
this is  an api change and the window is closed for go1.2.
please fine an issue and let's discuss after go1.2.

[rsc, 2013-09-23 20:16:25]

https://codereview.appspot.com/13457058/diff/11001/src/pkg/net/unixsock_posix.go
File src/pkg/net/unixsock_posix.go (right):

https://codereview.appspot.com/13457058/diff/11001/src/pkg/net/unixsock_posix...
src/pkg/net/unixsock_posix.go:274: return &UnixListener{fd, fd.laddr.String()},
nil
On 2013/09/19 19:58:28, mikio wrote:
> this is  an api change and the window is closed for go1.2.
> please fine an issue and let's discuss after go1.2.

In what way is this an API change? This looks like a bug fix to me. If laddr has
an empty path, then recording the empty path does not help anything. It will
make Close try to remove the empty string, and other associated bugs.
This looks fine to me. It also matches the behavior for TCP and UDP.

Mikio, can you explain why it's not okay for Go 1.2?

[mikio, 2013-09-23 23:31:19]

On Tue, Sep 24, 2013 at 5:16 AM,  <rsc@golang.org> wrote:

> In what way is this an API change? This looks like a bug fix to me.

Neverrmind. I misunderstood that this is a unixgram-specific feature.

[rsc, 2013-09-24 02:27:15]

LGTM

[rsc, 2013-09-24 02:33:49]

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

net: fix Close of autobind unix listener

Fixes issue 6455.

R=mikioh.mikioh, rsc
CC=golang-dev
https://codereview.appspot.com/13457058

Committer: Russ Cox <rsc@golang.org>