code review 11987043: net: fix memory leaks on windows

net: fix memory leaks on windows
Close netpoll descriptor along with socket.
Ensure that error paths close the descriptor as well.

[dvyukov, 2013-07-28 12:40:25]

Hello golang-dev@googlegroups.com,

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

[mikio, 2013-07-28 15:16:38]

https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:45: func closesocket(s syscall.Handle) error {
is this obsoleted?

[dvyukov, 2013-07-28 15:29:18]

https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:45: func closesocket(s syscall.Handle) error {
On 2013/07/28 15:16:38, mikio wrote:
> is this obsoleted?

It's still used in 2 places: netFD.Close() and if newFD() fails. But, yeah, I
can inline it, if that's what you mean.

[mikio, 2013-07-28 22:00:26]

On Mon, Jul 29, 2013 at 12:29 AM,  <dvyukov@google.com> wrote:

> It's still used in 2 places: netFD.Close() and if newFD() fails. But,
> yeah, I can inline it, if that's what you mean.

yup, let's collapse it if possible.

[brainman, 2013-07-29 04:08:38]

LGTM

but following comments.

Alex

https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:291: runtime.SetFinalizer(fd, (*netFD).Close)
But unix version does not call (*netFD).Close, but just closes file descriptor.
I changed that code recently https://codereview.appspot.com/6873046. So it means
unix version would be leaking memory too when finaliser is used. Also logically
you would not have any outstanding IO here (no one has access to this object
once in finaliser) so fd.pd.Close + closesocket should do here.

[dvyukov, 2013-07-29 07:36:18]

On Mon, Jul 29, 2013 at 8:08 AM,  <alex.brainman@gmail.com> wrote:
> LGTM
>
> but following comments.
>
> Alex
>
>
>
> https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go
> File src/pkg/net/fd_windows.go (right):
>
>
https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go#n...
> src/pkg/net/fd_windows.go:291: runtime.SetFinalizer(fd, (*netFD).Close)
> But unix version does not call (*netFD).Close, but just closes file
> descriptor. I changed that code recently
> https://codereview.appspot.com/6873046. So it means unix version would
> be leaking memory too when finaliser is used.

oops

> Also logically you would
> not have any outstanding IO here (no one has access to this object once
> in finaliser) so fd.pd.Close + closesocket should do here.

yeah, but this is exactly the approach that caused this bug, the leak
in unix finalizer, and some previous similar issues -- the approach
that you can have several versions of close for different situations
(that different versions eventually become inconsistent). A user would
have called Close(), and so must the finalizer.

[brainman, 2013-07-29 07:41:16]

On 2013/07/29 07:36:18, dvyukov wrote:
> ... A user would
> have called Close(), and so must the finalizer.

I am not expert enough here. Take it up with Russ. :-)

On the other hand, my missing of fd.pd.Close() should be fixed regardless.

Alex

[dvyukov, 2013-07-29 16:00:59]

https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go
File src/pkg/net/fd_windows.go (right):

https://codereview.appspot.com/11987043/diff/5001/src/pkg/net/fd_windows.go#n...
src/pkg/net/fd_windows.go:45: func closesocket(s syscall.Handle) error {
On 2013/07/28 15:29:19, dvyukov wrote:
> On 2013/07/28 15:16:38, mikio wrote:
> > is this obsoleted?
> 
> It's still used in 2 places: netFD.Close() and if newFD() fails. But, yeah, I
> can inline it, if that's what you mean.

This closesocket is also used in sock_posix.go which is shared between windows
and linux.
But I've removed:
func (fd *netFD) closesocket() error
which is not used.

[dvyukov, 2013-07-29 16:01:29]

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

net: fix memory leaks on windows
Close netpoll descriptor along with socket.
Ensure that error paths close the descriptor as well.

R=golang-dev, mikioh.mikioh, alex.brainman
CC=golang-dev
https://codereview.appspot.com/11987043