Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1571)

Issue 8318044: code review 8318044: net: fix possible runtime.PollDesc leak when connect or... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 12 months ago by mikio
Modified:
10 years, 11 months ago
Reviewers:
dfc, dvyukov, bradfitz
CC:
dfc, bradfitz, adg, golang-dev
Visibility:
Public.

Description

net: fix possible runtime.PollDesc leak when connect or listen fails Makes it possible to return the spent runtime.PollDesc to runtime.pollcache descriptor pool when netFD.connect or syscall.Listen fails. Fixes issue 5219.

Patch Set 1 #

Patch Set 2 : diff -r 3346bb37412c https://code.google.com/p/go #

Patch Set 3 : diff -r 3346bb37412c https://code.google.com/p/go #

Patch Set 4 : diff -r 3346bb37412c https://code.google.com/p/go #

Total comments: 2

Patch Set 5 : diff -r 1d49ee511d95 https://code.google.com/p/go #

Total comments: 4

Patch Set 6 : diff -r 8633d8460c82 https://code.google.com/p/go #

Patch Set 7 : diff -r b7fb7733b29b https://code.google.com/p/go #

Total comments: 6

Patch Set 8 : diff -r 72f34609fb25 https://code.google.com/p/go #

Total comments: 9

Patch Set 9 : diff -r 6b6253f60ef9 https://code.google.com/p/go #

Patch Set 10 : diff -r f2a65f5804ec https://code.google.com/p/go #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -6 lines) Patch
M src/pkg/net/fd_unix.go View 1 2 3 4 5 6 7 1 chunk +7 lines, -3 lines 1 comment Download
M src/pkg/net/sock_posix.go View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/net/tcpsock_posix.go View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/net/unixsock_posix.go View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 46
mikio
Hello dvyukov@google.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
10 years, 12 months ago (2013-04-05 05:38:04 UTC) #1
dvyukov
Ouch! Please add a test for this. OS tests somehow examine number of open fds, ...
10 years, 12 months ago (2013-04-05 16:30:12 UTC) #2
dvyukov
https://codereview.appspot.com/8318044/diff/4004/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/8318044/diff/4004/src/pkg/net/fd_unix.go#newcode136 src/pkg/net/fd_unix.go:136: func (fd *netFD) close() error { Why can't we ...
10 years, 12 months ago (2013-04-05 16:30:19 UTC) #3
dvyukov
Filed https://code.google.com/p/go/issues/detail?id=5219 Please add "Fixes issue 5219".
10 years, 12 months ago (2013-04-05 16:32:51 UTC) #4
dfc
Thanks Mikio, I've been trying to to review this CL but am concerned over the ...
10 years, 12 months ago (2013-04-05 22:24:28 UTC) #5
bradfitz
Is this a real leak or would the finalizer eventually get it? It should be ...
10 years, 12 months ago (2013-04-05 23:42:44 UTC) #6
mikio
https://codereview.appspot.com/8318044/diff/4004/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/8318044/diff/4004/src/pkg/net/fd_unix.go#newcode136 src/pkg/net/fd_unix.go:136: func (fd *netFD) close() error { On 2013/04/05 16:30:19, ...
10 years, 11 months ago (2013-04-06 07:42:12 UTC) #7
mikio
On 2013/04/05 22:24:28, dfc wrote: > 1, documented in a comment (possibly in fd_{unix,windows}.go#Close()) Will ...
10 years, 11 months ago (2013-04-06 07:53:54 UTC) #8
mikio
On 2013/04/05 23:42:44, bradfitz wrote: > Is this a real leak or would the finalizer ...
10 years, 11 months ago (2013-04-06 08:08:51 UTC) #9
mikio
On 2013/04/05 16:30:12, dvyukov wrote: > Please add a test for this. OS tests somehow ...
10 years, 11 months ago (2013-04-06 08:11:32 UTC) #10
mikio
Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 11 months ago (2013-04-06 10:41:21 UTC) #11
mikio
>> Please add a test for this. OS tests somehow examine number of open fds, ...
10 years, 11 months ago (2013-04-06 10:49:03 UTC) #12
bradfitz
Surely you can count something and see it rising or not rising in a test? ...
10 years, 11 months ago (2013-04-06 19:33:34 UTC) #13
dvyukov
https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/fd_windows.go File src/pkg/net/fd_windows.go (right): https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/fd_windows.go#newcode436 src/pkg/net/fd_windows.go:436: // integrated poll descriptor. Explain that "and an associated ...
10 years, 11 months ago (2013-04-07 04:27:38 UTC) #14
mikio
https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/fd_windows.go File src/pkg/net/fd_windows.go (right): https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/fd_windows.go#newcode436 src/pkg/net/fd_windows.go:436: // integrated poll descriptor. On 2013/04/07 04:27:39, dvyukov wrote: ...
10 years, 11 months ago (2013-04-07 15:10:26 UTC) #15
mikio
Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 11 months ago (2013-04-07 15:10:58 UTC) #16
dvyukov
On 2013/04/07 15:10:26, mikio wrote: > https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/sock_posix.go#newcode61 > src/pkg/net/sock_posix.go:61: fd.closeSocket() > On 2013/04/07 04:27:39, dvyukov ...
10 years, 11 months ago (2013-04-07 22:01:04 UTC) #17
mikio
PTAL (and a bit late to say, but thanks for fixing runtime stuff!) > But ...
10 years, 11 months ago (2013-04-08 05:00:22 UTC) #18
adg
How critical is it that this gets into the 1.1 release? And how risky is ...
10 years, 11 months ago (2013-04-08 06:43:42 UTC) #19
dfc
I think we are arguing about style, not substance. Please allow me to review this ...
10 years, 11 months ago (2013-04-08 06:44:41 UTC) #20
dfc
LGTM. Thank you, sorry this took so long. After 1.1 ships, I'd like to see ...
10 years, 11 months ago (2013-04-08 07:19:28 UTC) #21
dfc
As a followup, prior to this CL, this short program would leak memory at a ...
10 years, 11 months ago (2013-04-08 07:44:30 UTC) #22
dvyukov
On Sun, Apr 7, 2013 at 11:43 PM, <adg@golang.org> wrote: > How critical is it ...
10 years, 11 months ago (2013-04-08 15:06:27 UTC) #23
bradfitz
Then there's our test. I thought a test would be difficult? On Apr 8, 2013 ...
10 years, 11 months ago (2013-04-08 16:30:10 UTC) #24
dfc
I think it is difficult to observe the leak inside a Go program as the ...
10 years, 11 months ago (2013-04-09 00:07:24 UTC) #25
bradfitz
There's always syscall.Getrusage like I did in test/mapnan.go On Mon, Apr 8, 2013 at 5:07 ...
10 years, 11 months ago (2013-04-09 00:15:39 UTC) #26
dvyukov
On Mon, Apr 8, 2013 at 5:07 PM, Dave Cheney <dave@cheney.net> wrote: > I think ...
10 years, 11 months ago (2013-04-09 00:24:50 UTC) #27
dfc
Here is my attempt at a test, https://codereview.appspot.com/8547043 It detects the leak on my system ...
10 years, 11 months ago (2013-04-09 00:55:51 UTC) #28
bradfitz
LGTM Update CL description to say the word "Issue" before the number ("Update Issue nnnnn") ...
10 years, 11 months ago (2013-04-09 01:03:28 UTC) #29
bradfitz
LGTM for Dave's test, that is. On Mon, Apr 8, 2013 at 6:03 PM, Brad ...
10 years, 11 months ago (2013-04-09 01:03:50 UTC) #30
mikio
PTAL Thank you for adding a new test, Dave. But it doesn't work well on ...
10 years, 11 months ago (2013-04-09 02:06:33 UTC) #31
dfc
> i'll leave it to you. > feel free to send a cl at the ...
10 years, 11 months ago (2013-04-09 02:11:09 UTC) #32
bradfitz
https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go File src/pkg/net/dial_test.go (right): https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go#newcode344 src/pkg/net/dial_test.go:344: const attempts = 60000 // poll descriptor pool grows ...
10 years, 11 months ago (2013-04-09 02:11:25 UTC) #33
dfc
LGTM. Thank you for making my test more robust -- leaving for bradfitz. https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go File ...
10 years, 11 months ago (2013-04-09 02:17:30 UTC) #34
mikio
https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go File src/pkg/net/dial_test.go (right): https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go#newcode344 src/pkg/net/dial_test.go:344: const attempts = 60000 // poll descriptor pool grows ...
10 years, 11 months ago (2013-04-09 02:26:43 UTC) #35
mikio
Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 11 months ago (2013-04-09 02:27:15 UTC) #36
mikio
sorry, my test (or perhaps my vm) is still broken, can not get consistent results. ...
10 years, 11 months ago (2013-04-09 03:25:26 UTC) #37
dfc
SGTM. Maybe only run the test on platforms that have the new poller ? On ...
10 years, 11 months ago (2013-04-09 03:29:33 UTC) #38
bradfitz
Or just on Linux. For lots of tests like this just catching a rare regression, ...
10 years, 11 months ago (2013-04-09 03:32:00 UTC) #39
dfc
Even better, make it so. On Tue, Apr 9, 2013 at 1:31 PM, Brad Fitzpatrick ...
10 years, 11 months ago (2013-04-09 03:35:19 UTC) #40
mikio
On Tue, Apr 9, 2013 at 12:29 PM, Dave Cheney <dave@cheney.net> wrote: > SGTM. Maybe ...
10 years, 11 months ago (2013-04-09 03:39:33 UTC) #41
mikio
yup, will do so in go 1.2. On Tue, Apr 9, 2013 at 12:31 PM, ...
10 years, 11 months ago (2013-04-09 03:40:20 UTC) #42
mikio
*** Submitted as https://code.google.com/p/go/source/detail?r=c40f15e9e5ca *** net: fix possible runtime.PollDesc leak when connect or listen fails ...
10 years, 11 months ago (2013-04-09 03:42:06 UTC) #43
dvyukov
LGTM
10 years, 11 months ago (2013-04-09 04:03:47 UTC) #44
dvyukov
https://codereview.appspot.com/8318044/diff/55006/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/8318044/diff/55006/src/pkg/net/fd_unix.go#newcode128 src/pkg/net/fd_unix.go:128: fd.pd.Close() This can not be called twice, right?
10 years, 11 months ago (2013-04-09 04:07:47 UTC) #45
mikio
10 years, 11 months ago (2013-04-09 04:36:29 UTC) #46
On Tue, Apr 9, 2013 at 1:07 PM,  <dvyukov@google.com> wrote:

> src/pkg/net/fd_unix.go:128: fd.pd.Close()
> This can not be called twice, right?

Right. A combo of sysref and closing protects it.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b