code review 91230045: net/rpc: do not leak client socket on closed connection

net/rpc: do not leak client socket on closed connection

Fixes issue 6897.

[crawshaw, 2014-05-07 18:36:39]

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

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

[bradfitz, 2014-05-07 18:53:34]

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client.go
File src/pkg/net/rpc/client.go (right):

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client.go#...
src/pkg/net/rpc/client.go:48: closing  bool
document these two fields?

the fix isn't obvious at least.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
File src/pkg/net/rpc/client_test.go (right):

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:50: time.Sleep(20 * time.Millisecond)
how long does this test take? If long, t.Skip if testing.Short() and say
skipping during short test.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:75: if got, want := len(finalFDs), len(initFDs);
got != want {
!reflect.DeepEqual?

but these tests are likely flaky if background goroutines are doing something.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:82: var procfd = fmt.Sprintf("/proc/%d/fd",
os.Getpid())
see the os/exec tests for how to do this more portably with lsof.

but this is also okay with the skip.  I would explicitly skip for both failure
to open the directory (how you have it) and also on runtime.GOOS != "linux", in
case a non-Linux does /proc/../fd differently.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:85: // use ls -l instead of f.Readdirnames so we
can see the symlinked names
Could just use Readlink, but this works I guess since you skip on failure.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:107: // BenchmarkCloseClientServer is not a
benchmark.
The typical way to do this isn't to make a new Test or Benchmark, but to instead
use TestFoo as both client and child process, depending on an environment
variable.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:137: read int
written?

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:148: func (l *lockedWriter) BytesRead() int {
BytesWitten, no?

[crawshaw, 2014-05-07 19:11:47]

PTAL

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client.go
File src/pkg/net/rpc/client.go (right):

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client.go#...
src/pkg/net/rpc/client.go:48: closing  bool
On 2014/05/07 18:53:33, bradfitz wrote:
> document these two fields?
> 
> the fix isn't obvious at least.

done, as best i understand it.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
File src/pkg/net/rpc/client_test.go (right):

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:50: time.Sleep(20 * time.Millisecond)
On 2014/05/07 18:53:34, bradfitz wrote:
> how long does this test take? If long, t.Skip if testing.Short() and say
> skipping during short test.

the first sleep is plenty on my machine, i just gave it lots of headroom for
tiny/slow machines.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:75: if got, want := len(finalFDs), len(initFDs);
got != want {
On 2014/05/07 18:53:34, bradfitz wrote:
> !reflect.DeepEqual?
> 
> but these tests are likely flaky if background goroutines are doing something.

even better, compared to none.

i'm filtering on the descriptor having the name "socket:...", so there's nothing
else to see.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:82: var procfd = fmt.Sprintf("/proc/%d/fd",
os.Getpid())
On 2014/05/07 18:53:33, bradfitz wrote:
> see the os/exec tests for how to do this more portably with lsof.
> 
> but this is also okay with the skip.  I would explicitly skip for both failure
> to open the directory (how you have it) and also on runtime.GOOS != "linux",
in
> case a non-Linux does /proc/../fd differently.

Done.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:85: // use ls -l instead of f.Readdirnames so we
can see the symlinked names
On 2014/05/07 18:53:34, bradfitz wrote:
> Could just use Readlink, but this works I guess since you skip on failure.

lsof it is

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:107: // BenchmarkCloseClientServer is not a
benchmark.
On 2014/05/07 18:53:34, bradfitz wrote:
> The typical way to do this isn't to make a new Test or Benchmark, but to
instead
> use TestFoo as both client and child process, depending on an environment
> variable.

Done, nice.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:137: read int
On 2014/05/07 18:53:33, bradfitz wrote:
> written?

Done.

https://codereview.appspot.com/91230045/diff/40001/src/pkg/net/rpc/client_tes...
src/pkg/net/rpc/client_test.go:148: func (l *lockedWriter) BytesRead() int {
On 2014/05/07 18:53:33, bradfitz wrote:
> BytesWitten, no?

Done.

[bradfitz, 2014-05-07 19:23:46]

LGTM but wait for r

https://codereview.appspot.com/91230045/diff/80001/src/pkg/net/rpc/client.go
File src/pkg/net/rpc/client.go (right):

https://codereview.appspot.com/91230045/diff/80001/src/pkg/net/rpc/client.go#...
src/pkg/net/rpc/client.go:42: mutex    sync.Mutex // protects pending, seq,
request
I think this protects more than those 3. Maybe all but codec? Or all?

Update comment ("protects following") and/or rearrange non-guarded things above
the mutex?

[crawshaw, 2014-05-07 19:34:32]

https://codereview.appspot.com/91230045/diff/80001/src/pkg/net/rpc/client.go
File src/pkg/net/rpc/client.go (right):

https://codereview.appspot.com/91230045/diff/80001/src/pkg/net/rpc/client.go#...
src/pkg/net/rpc/client.go:42: mutex    sync.Mutex // protects pending, seq,
request
On 2014/05/07 19:23:46, bradfitz wrote:
> I think this protects more than those 3. Maybe all but codec? Or all?
> 
> Update comment ("protects following") and/or rearrange non-guarded things
above
> the mutex?

Done.

[bradfitz, 2014-05-07 22:37:32]

LGTM



On Wed, May 7, 2014 at 12:34 PM, <crawshaw@golang.org> wrote:

>
> https://codereview.appspot.com/91230045/diff/80001/src/
> pkg/net/rpc/client.go
> File src/pkg/net/rpc/client.go (right):
>
> https://codereview.appspot.com/91230045/diff/80001/src/
> pkg/net/rpc/client.go#newcode42
> src/pkg/net/rpc/client.go:42: mutex    sync.Mutex // protects pending,
> seq, request
> On 2014/05/07 19:23:46, bradfitz wrote:
>
>> I think this protects more than those 3. Maybe all but codec? Or all?
>>
>
>  Update comment ("protects following") and/or rearrange non-guarded
>>
> things above
>
>> the mutex?
>>
>
> Done.
>
> https://codereview.appspot.com/91230045/
>

[rsc, 2014-05-11 18:29:40]

The fix is fine. The test seems like overkill. You get to specify the Codec
implementation. You should be able to tell whether Close was called without
needing a subprocess and lsof.

[crawshaw, 2014-05-11 21:39:09]

On 2014/05/11 18:29:40, rsc wrote:
> The fix is fine. The test seems like overkill. You get to specify the Codec
> implementation. You should be able to tell whether Close was called without
> needing a subprocess and lsof.

Thanks, that's much better. PTAL.

[bradfitz, 2014-05-11 21:45:01]

LGTM



On Sun, May 11, 2014 at 2:39 PM, <crawshaw@golang.org> wrote:

> On 2014/05/11 18:29:40, rsc wrote:
>
>> The fix is fine. The test seems like overkill. You get to specify the
>>
> Codec
>
>> implementation. You should be able to tell whether Close was called
>>
> without
>
>> needing a subprocess and lsof.
>>
>
> Thanks, that's much better. PTAL.
>
> https://codereview.appspot.com/91230045/
>

[bradfitz, 2014-05-11 21:46:54]

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

net/rpc: do not leak client socket on closed connection

Fixes issue 6897.

LGTM=bradfitz
R=golang-codereviews, bradfitz, r, rsc
CC=golang-codereviews
https://codereview.appspot.com/91230045

Committer: Brad Fitzpatrick <bradfitz@golang.org>