Issue 8318044: code review 8318044: net: fix possible runtime.PollDesc leak when connect or... (Closed)
Descriptionnet: 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
MessagesTotal messages: 46
Hello dvyukov@google.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Ouch! Please add a test for this. OS tests somehow examine number of open fds, it should now grow after failed connect/listen. Or perhaps do a lots of them and see for ENFILE.
Sign in to reply to this message.
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#newco... src/pkg/net/fd_unix.go:136: func (fd *netFD) close() error { Why can't we use Close() instead of close()? As far as I see it would do the same.
Sign in to reply to this message.
Filed https://code.google.com/p/go/issues/detail?id=5219 Please add "Fixes issue 5219".
Sign in to reply to this message.
Thanks Mikio,
I've been trying to to review this CL but am concerned over the
explosion of things to close and the number of the ways they are
closed. Can the close sequence be,
1, documented in a comment (possibly in fd_{unix,windows}.go#Close())
2, converged towards a single code path.
I think the outliers are the close paths on the variuos Listen
methods. Right now I am leaning towards a comment block and en
explicit invocation of all the parts, ie fd.pd.Evict(), fd.pd.close(),
etc.
On Sat, Apr 6, 2013 at 3:32 AM, <dvyukov@google.com> wrote:
> Filed https://code.google.com/p/go/issues/detail?id=5219
> Please add "Fixes issue 5219".
>
>
>
> https://codereview.appspot.com/8318044/
>
> --
>
> ---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.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
Sign in to reply to this message.
Is this a real leak or would the finalizer eventually get it? It should be fixed in either case, of course, but I'm wondering how critical it is for Go1.1. Mikioh--- please file an issue with more details regardless. On Fri, Apr 5, 2013 at 9:30 AM, <dvyukov@google.com> wrote: > Ouch! > > Please add a test for this. OS tests somehow examine number of open fds, > it should now grow after failed connect/listen. Or perhaps do a lots of > them and see for ENFILE. > > > https://codereview.appspot.**com/8318044/<https://codereview.appspot.com/8318... > > -- > > ---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... > . > > >
Sign in to reply to this message.
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#newco... src/pkg/net/fd_unix.go:136: func (fd *netFD) close() error { On 2013/04/05 16:30:19, dvyukov wrote: > Why can't we use Close() instead of close()? > As far as I see it would do the same. I just don't want to break Rémy's changeset: 15019:6ec24fe2e501.
Sign in to reply to this message.
On 2013/04/05 22:24:28, dfc wrote:
> 1, documented in a comment (possibly in fd_{unix,windows}.go#Close())
Will add a bit.
> 2, converged towards a single code path.
If once we land on the concrete poller stuff, but not now.
Sign in to reply to this message.
On 2013/04/05 23:42:44, bradfitz wrote: > Is this a real leak or would the finalizer eventually get it? Perhaps it's a real, I guess. netFD.setAddr sets up finalizer but this happens btw newFD and netFD.setAddr. > It should be fixed in either case, of course, but I'm wondering how > critical it is for Go1.1.
Sign in to reply to this message.
On 2013/04/05 16:30:12, dvyukov wrote: > Please add a test for this. OS tests somehow examine number of open fds, it > should now grow after failed connect/listen. Or perhaps do a lots of them and > see for ENFILE. yup.
Sign in to reply to this message.
Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
>> Please add a test for this. OS tests somehow examine number of open fds, it >> should now grow after failed connect/listen. Or perhaps do a lots of them and >> see for ENFILE. > > yup. sorry i misunderstood. the problem is not a file descriptor leak, it's a simple memory leak. do you need a test for it? it might be a long-run test.
Sign in to reply to this message.
Surely you can count something and see it rising or not rising in a test? Anyway, leaving for Dmitry and Dave. On Sat, Apr 6, 2013 at 3:49 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > >> Please add a test for this. OS tests somehow examine number of open > fds, it > >> should now grow after failed connect/listen. Or perhaps do a lots of > them and > >> see for ENFILE. > > > > yup. > > sorry i misunderstood. the problem is not a file descriptor leak, > it's a simple memory leak. do you need a test for it? it might be > a long-run test. >
Sign in to reply to this message.
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#n... src/pkg/net/fd_windows.go:436: // integrated poll descriptor. Explain that "and an associated runtime integrated poll descriptor" is a no-op, because the integrated poll is not yet implemented on windows. Otherwise it's confusing. https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (right): https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/sock_posix.go#n... src/pkg/net/sock_posix.go:61: fd.closeSocket() Why do you use closeSocket() here and Close() in listen? Don't they do the same thing?
Sign in to reply to this message.
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#n... src/pkg/net/fd_windows.go:436: // integrated poll descriptor. On 2013/04/07 04:27:39, dvyukov wrote: > Explain that "and an associated runtime integrated poll descriptor" is a no-op, > because the integrated poll is not yet implemented on windows. > Otherwise it's confusing. Done. https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/sock_posix.go File src/pkg/net/sock_posix.go (right): https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/sock_posix.go#n... src/pkg/net/sock_posix.go:61: fd.closeSocket() On 2013/04/07 04:27:39, dvyukov wrote: > Why do you use closeSocket() here and Close() in listen? > Don't they do the same thing? Nope, closeSocket is just a closer for a larval phase netFD before finalizer is settled, and Close is for a hatched one.
Sign in to reply to this message.
Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2013/04/07 15:10:26, mikio wrote: > https://codereview.appspot.com/8318044/diff/19001/src/pkg/net/sock_posix.go#n... > src/pkg/net/sock_posix.go:61: fd.closeSocket() > On 2013/04/07 04:27:39, dvyukov wrote: > > Why do you use closeSocket() here and Close() in listen? > > Don't they do the same thing? > > Nope, closeSocket is just a closer for a larval phase netFD > before finalizer is settled, and Close is for a hatched one. But the net effect is the same, right? I would use the following invariant: if newFD() returns a non-nil netFD, it always must be closed with Close(). netFD implementation can handle own phases (if any) internally. This also brings another question: now netFD contains another non-managed resource (runtime PollDesc). Do we want to set finalizers for netFD to release them? What is the practice of using os.File/net.Conn w/o closing them? I would expect it to broke sooner or later anyway. Sooner is better.
Sign in to reply to this message.
PTAL (and a bit late to say, but thanks for fixing runtime stuff!) > But the net effect is the same, right? > I would use the following invariant: if newFD() returns a non-nil netFD, it > always must be closed with Close(). netFD implementation can handle own phases > (if any) internally. That is Dave mentioned previously and I said not now because anyway we have to tweak such code paths for implementing new poller to BSD variants in Go 1.2 development. But both of you say so, okay, I changed my mind, will do so. > This also brings another question: now netFD contains another non-managed > resource (runtime PollDesc). Do we want to set finalizers for netFD to release > them? What is the practice of using os.File/net.Conn w/o closing them? I would > expect it to broke sooner or later anyway. Sooner is better. Finalizer is good as a side bet (an insurance) and providing explicit closer is also nice. What's the matter? Are you suggesting that eventually we should take Finalizer out of netFD?
Sign in to reply to this message.
How critical is it that this gets into the 1.1 release? And how risky is its inclusion? I want to issue a 1.1 release candidate in the next couple of days.
Sign in to reply to this message.
I think we are arguing about style, not substance. Please allow me to review this CL again tonight, I believe it is important enough to warrant inclusion for the release candidate. On Mon, Apr 8, 2013 at 4:43 PM, <adg@golang.org> wrote: > How critical is it that this gets into the 1.1 release? And how risky is > its inclusion? > > I want to issue a 1.1 release candidate in the next couple of days. > > https://codereview.appspot.com/8318044/
Sign in to reply to this message.
LGTM. Thank you, sorry this took so long. After 1.1 ships, I'd like to see the shared responsibility for the underlying fd that is currently split between fd.sysfd and fd.sysfile revisited. https://codereview.appspot.com/8318044/diff/36001/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/8318044/diff/36001/src/pkg/net/fd_unix.go#newc... src/pkg/net/fd_unix.go:121: // (and there are no references left.) please revert this documentation change, just close the bracket on the second line. https://codereview.appspot.com/8318044/diff/36001/src/pkg/net/fd_unix.go#newc... src/pkg/net/fd_unix.go:125: if fd.closing && fd.sysref == 0 { in the previous version fd.sysfile != nil was the final assertion that prevented the closing behavior from firing twice. Now we have falled back to fd.sysref == 0, which I think is safe, but makes me feel a little uncomfortable. https://codereview.appspot.com/8318044/diff/36001/src/pkg/net/fd_unix.go#newc... src/pkg/net/fd_unix.go:132: } else { could you please add a comment explaining why fd.sysfile is nil, something like // fd.sysfile can be nil if fd.Close() was called from socket() before // the socket was fully created. https://codereview.appspot.com/8318044/diff/36001/src/pkg/net/fd_windows.go File src/pkg/net/fd_windows.go (right): https://codereview.appspot.com/8318044/diff/36001/src/pkg/net/fd_windows.go#n... src/pkg/net/fd_windows.go:418: // (and there are no references left.) ditto
Sign in to reply to this message.
As a followup, prior to this CL, this short program would leak memory at a
frightening rate
package main
import (
"net"
)
func main() {
for i := 0; ; i++ {
conn, err := net.Dial("tcp", "localhost:1")
if err != nil {
continue
}
conn.Close()
}
}
Sign in to reply to this message.
On Sun, Apr 7, 2013 at 11:43 PM, <adg@golang.org> wrote: > How critical is it that this gets into the 1.1 release? I think it is. Because failed TCP connect leaks memory. Some servers will crash periodically. > And how risky is > its inclusion? > > I want to issue a 1.1 release candidate in the next couple of days. > > https://codereview.appspot.**com/8318044/<https://codereview.appspot.com/8318... >
Sign in to reply to this message.
Then there's our test. I thought a test would be difficult? On Apr 8, 2013 12:44 AM, <dave@cheney.net> wrote: > As a followup, prior to this CL, this short program would leak memory at > a frightening rate > > package main > > import ( > "net" > ) > > func main() { > for i := 0; ; i++ { > conn, err := net.Dial("tcp", "localhost:1") > if err != nil { > continue > } > conn.Close() > } > } > > > > https://codereview.appspot.**com/8318044/<https://codereview.appspot.com/8318... >
Sign in to reply to this message.
I think it is difficult to observe the leak inside a Go program as the runtime.PollDesc allocations do not go through mallogc (Dmitry, is this correct?). For example, compare the output of running this program in GOGCTRACE=1 to the output of top. On Tue, Apr 9, 2013 at 2:30 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Then there's our test. > > I thought a test would be difficult? > > On Apr 8, 2013 12:44 AM, <dave@cheney.net> wrote: >> >> As a followup, prior to this CL, this short program would leak memory at >> a frightening rate >> >> package main >> >> import ( >> "net" >> ) >> >> func main() { >> for i := 0; ; i++ { >> conn, err := net.Dial("tcp", "localhost:1") >> if err != nil { >> continue >> } >> conn.Close() >> } >> } >> >> >> >> https://codereview.appspot.com/8318044/ > > -- > > --- > 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. > For more options, visit https://groups.google.com/groups/opt_out. > >
Sign in to reply to this message.
There's always syscall.Getrusage like I did in test/mapnan.go On Mon, Apr 8, 2013 at 5:07 PM, Dave Cheney <dave@cheney.net> wrote: > I think it is difficult to observe the leak inside a Go program as the > runtime.PollDesc allocations do not go through mallogc (Dmitry, is > this correct?). For example, compare the output of running this > program in GOGCTRACE=1 to the output of top. > > On Tue, Apr 9, 2013 at 2:30 AM, Brad Fitzpatrick <bradfitz@golang.org> > wrote: > > Then there's our test. > > > > I thought a test would be difficult? > > > > On Apr 8, 2013 12:44 AM, <dave@cheney.net> wrote: > >> > >> As a followup, prior to this CL, this short program would leak memory at > >> a frightening rate > >> > >> package main > >> > >> import ( > >> "net" > >> ) > >> > >> func main() { > >> for i := 0; ; i++ { > >> conn, err := net.Dial("tcp", "localhost:1") > >> if err != nil { > >> continue > >> } > >> conn.Close() > >> } > >> } > >> > >> > >> > >> https://codereview.appspot.com/8318044/ > > > > -- > > > > --- > > 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. > > For more options, visit https://groups.google.com/groups/opt_out. > > > > >
Sign in to reply to this message.
On Mon, Apr 8, 2013 at 5:07 PM, Dave Cheney <dave@cheney.net> wrote: > I think it is difficult to observe the leak inside a Go program as the > runtime.PollDesc allocations do not go through mallogc (Dmitry, is > this correct?). For example, compare the output of running this > program in GOGCTRACE=1 to the output of top. Yes, that's correct. But I think it should be accounted in MemStats in Sys/SysOther. > On Tue, Apr 9, 2013 at 2:30 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote: >> Then there's our test. >> >> I thought a test would be difficult? >> >> On Apr 8, 2013 12:44 AM, <dave@cheney.net> wrote: >>> >>> As a followup, prior to this CL, this short program would leak memory at >>> a frightening rate >>> >>> package main >>> >>> import ( >>> "net" >>> ) >>> >>> func main() { >>> for i := 0; ; i++ { >>> conn, err := net.Dial("tcp", "localhost:1") >>> if err != nil { >>> continue >>> } >>> conn.Close() >>> } >>> } >>> >>> >>> >>> https://codereview.appspot.com/8318044/ >> >> -- >> >> --- >> 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. >> For more options, visit https://groups.google.com/groups/opt_out. >> >>
Sign in to reply to this message.
Here is my attempt at a test, https://codereview.appspot.com/8547043 It detects the leak on my system but I am not sure how stable it will be across all the platforms we support.
Sign in to reply to this message.
LGTM
Update CL description to say the word "Issue" before the number ("Update
Issue
nnnnn") and then add a t.Skip line at the top. The fix commit later can
remove
the line.
On Mon, Apr 8, 2013 at 5:55 PM, <dave@cheney.net> wrote:
> Here is my attempt at a test,
>
>
https://codereview.appspot.**com/8547043<https://codereview.appspot.com/8547043>
>
> It detects the leak on my system but I am not sure how stable it will be
> across all the platforms we support.
>
>
https://codereview.appspot.**com/8318044/<https://codereview.appspot.com/8318...
>
Sign in to reply to this message.
LGTM for Dave's test, that is. On Mon, Apr 8, 2013 at 6:03 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > LGTM > > Update CL description to say the word "Issue" before the number ("Update > Issue > nnnnn") and then add a t.Skip line at the top. The fix commit later can > remove > the line. > > > > On Mon, Apr 8, 2013 at 5:55 PM, <dave@cheney.net> wrote: > >> Here is my attempt at a test, >> >> https://codereview.appspot.**com/8547043<https://codereview.appspot.com/8547043> >> >> It detects the leak on my system but I am not sure how stable it will be >> across all the platforms we support. >> >> https://codereview.appspot.**com/8318044/<https://codereview.appspot.com/8318... >> > >
Sign in to reply to this message.
PTAL Thank you for adding a new test, Dave. But it doesn't work well on my freebsd/386 vm sometimes, so I simplified it a bit. https://codereview.appspot.com/8318044/diff/36001/src/pkg/net/fd_unix.go File src/pkg/net/fd_unix.go (right): https://codereview.appspot.com/8318044/diff/36001/src/pkg/net/fd_unix.go#newc... src/pkg/net/fd_unix.go:121: // (and there are no references left.) On 2013/04/08 07:19:29, dfc wrote: > please revert this documentation change, just close the bracket on the second > line. i'll leave it to you. feel free to send a cl at the appropriate time, thx. https://codereview.appspot.com/8318044/diff/36001/src/pkg/net/fd_unix.go#newc... src/pkg/net/fd_unix.go:132: } else { On 2013/04/08 07:19:29, dfc wrote: > could you please add a comment explaining why fd.sysfile is nil, something like > > // fd.sysfile can be nil if fd.Close() was called from socket() before > // the socket was fully created. thanks but no thanks. it's obvious that that's a chicken or egg test.
Sign in to reply to this message.
> i'll leave it to you. > feel free to send a cl at the appropriate time, thx. Will do. > thanks but no thanks. > it's obvious that that's a chicken or egg test. SGTM. I'd like to see the close logic overhauled as part of the 1.2 development cycle.
Sign in to reply to this message.
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#ne... src/pkg/net/dial_test.go:344: const attempts = 60000 // poll descriptor pool grows linearly, so we need enough attempts to identfy memory leak identify https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go#ne... src/pkg/net/dial_test.go:346: c, err := (&Dialer{Timeout: time.Nanosecond}).Dial("tcp", "127.0.0.1:1") just make the dialer once above the loop and re-use it. https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go#ne... src/pkg/net/dial_test.go:357: case r < 1.3: pretty arbitrary? https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go#ne... src/pkg/net/dial_test.go:358: t.Logf("pretty moderate: %v", r) delete. weird message.
Sign in to reply to this message.
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 src/pkg/net/dial_test.go (right): https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go#ne... src/pkg/net/dial_test.go:346: c, err := (&Dialer{Timeout: time.Nanosecond}).Dial("tcp", "127.0.0.1:1") I think this is overkill, nothing is listening on 127.0.0.1:1
Sign in to reply to this message.
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#ne... src/pkg/net/dial_test.go:344: const attempts = 60000 // poll descriptor pool grows linearly, so we need enough attempts to identfy memory leak On 2013/04/09 02:11:25, bradfitz wrote: > identify Done. https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go#ne... src/pkg/net/dial_test.go:346: c, err := (&Dialer{Timeout: time.Nanosecond}).Dial("tcp", "127.0.0.1:1") On 2013/04/09 02:11:25, bradfitz wrote: > just make the dialer once above the loop and re-use it. Done. https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go#ne... src/pkg/net/dial_test.go:357: case r < 1.3: On 2013/04/09 02:11:25, bradfitz wrote: > pretty arbitrary? not precisely, but comes from measured values on a few platforms. alos when leak happens, the value would be greater than 3.x. https://codereview.appspot.com/8318044/diff/53001/src/pkg/net/dial_test.go#ne... src/pkg/net/dial_test.go:358: t.Logf("pretty moderate: %v", r) On 2013/04/09 02:11:25, bradfitz wrote: > delete. weird message. Done.
Sign in to reply to this message.
Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
sorry, my test (or perhaps my vm) is still broken, can not get consistent results. so i'll go with Dave's test for Go 1.1 and will add runtime_PollerStats or similars to collect poller's memory allocation. thanks. On Tue, Apr 9, 2013 at 11:27 AM, <mikioh.mikioh@gmail.com> wrote: > Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org, > adg@golang.org (cc: golang-dev@googlegroups.com), > > > Please take another look. > > > https://codereview.appspot.com/8318044/
Sign in to reply to this message.
SGTM. Maybe only run the test on platforms that have the new poller ? On Tue, Apr 9, 2013 at 1:25 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > sorry, my test (or perhaps my vm) is still broken, can not get > consistent results. > so i'll go with Dave's test for Go 1.1 and will add > runtime_PollerStats or similars > to collect poller's memory allocation. thanks. > > > On Tue, Apr 9, 2013 at 11:27 AM, <mikioh.mikioh@gmail.com> wrote: >> Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org, >> adg@golang.org (cc: golang-dev@googlegroups.com), >> >> >> Please take another look. >> >> >> https://codereview.appspot.com/8318044/
Sign in to reply to this message.
Or just on Linux. For lots of tests like this just catching a rare regression, having any builder fail (especially a popular one like darwin or linux) is sufficient. I'd rather have reliable OS-specific tests than flaky tests. On Mon, Apr 8, 2013 at 8:29 PM, Dave Cheney <dave@cheney.net> wrote: > SGTM. Maybe only run the test on platforms that have the new poller ? > > On Tue, Apr 9, 2013 at 1:25 PM, Mikio Hara <mikioh.mikioh@gmail.com> > wrote: > > sorry, my test (or perhaps my vm) is still broken, can not get > > consistent results. > > so i'll go with Dave's test for Go 1.1 and will add > > runtime_PollerStats or similars > > to collect poller's memory allocation. thanks. > > > > > > On Tue, Apr 9, 2013 at 11:27 AM, <mikioh.mikioh@gmail.com> wrote: > >> Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org, > >> adg@golang.org (cc: golang-dev@googlegroups.com), > >> > >> > >> Please take another look. > >> > >> > >> https://codereview.appspot.com/8318044/ >
Sign in to reply to this message.
Even better, make it so. On Tue, Apr 9, 2013 at 1:31 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Or just on Linux. > > For lots of tests like this just catching a rare regression, having any > builder fail (especially a popular one like darwin or linux) is sufficient. > I'd rather have reliable OS-specific tests than flaky tests. > > > > On Mon, Apr 8, 2013 at 8:29 PM, Dave Cheney <dave@cheney.net> wrote: >> >> SGTM. Maybe only run the test on platforms that have the new poller ? >> >> On Tue, Apr 9, 2013 at 1:25 PM, Mikio Hara <mikioh.mikioh@gmail.com> >> wrote: >> > sorry, my test (or perhaps my vm) is still broken, can not get >> > consistent results. >> > so i'll go with Dave's test for Go 1.1 and will add >> > runtime_PollerStats or similars >> > to collect poller's memory allocation. thanks. >> > >> > >> > On Tue, Apr 9, 2013 at 11:27 AM, <mikioh.mikioh@gmail.com> wrote: >> >> Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org, >> >> adg@golang.org (cc: golang-dev@googlegroups.com), >> >> >> >> >> >> Please take another look. >> >> >> >> >> >> https://codereview.appspot.com/8318044/ > >
Sign in to reply to this message.
On Tue, Apr 9, 2013 at 12:29 PM, Dave Cheney <dave@cheney.net> wrote: > SGTM. Maybe only run the test on platforms that have the new poller ? Not sure. Just 1hr ago it worked on both amd64 and 386 but right now it fails on 386. Probably we have an another issue on 386 platform.
Sign in to reply to this message.
yup, will do so in go 1.2. On Tue, Apr 9, 2013 at 12:31 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > Or just on Linux. > > For lots of tests like this just catching a rare regression, having any > builder fail (especially a popular one like darwin or linux) is sufficient. > I'd rather have reliable OS-specific tests than flaky tests. > > > > On Mon, Apr 8, 2013 at 8:29 PM, Dave Cheney <dave@cheney.net> wrote: >> >> SGTM. Maybe only run the test on platforms that have the new poller ? >> >> On Tue, Apr 9, 2013 at 1:25 PM, Mikio Hara <mikioh.mikioh@gmail.com> >> wrote: >> > sorry, my test (or perhaps my vm) is still broken, can not get >> > consistent results. >> > so i'll go with Dave's test for Go 1.1 and will add >> > runtime_PollerStats or similars >> > to collect poller's memory allocation. thanks. >> > >> > >> > On Tue, Apr 9, 2013 at 11:27 AM, <mikioh.mikioh@gmail.com> wrote: >> >> Hello dvyukov@google.com, dave@cheney.net, bradfitz@golang.org, >> >> adg@golang.org (cc: golang-dev@googlegroups.com), >> >> >> >> >> >> Please take another look. >> >> >> >> >> >> https://codereview.appspot.com/8318044/ > >
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=c40f15e9e5ca *** 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. R=dvyukov, dave, bradfitz, adg CC=golang-dev https://codereview.appspot.com/8318044
Sign in to reply to this message.
Message was sent while issue was closed.
LGTM
Sign in to reply to this message.
Message was sent while issue was closed.
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#newc... src/pkg/net/fd_unix.go:128: fd.pd.Close() This can not be called twice, right?
Sign in to reply to this message.
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.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
