[dart:io] Adds a finalizer to _NativeSocket to avoid socket leaks

[dart:io] Adds a finalizer to _NativeSocket to avoid socket leaks

The finalizer sends the "close" message to the EventHandler for the
file descriptor in the _NativeSocket's native field. To avoid races and
spurious messages, this CL stores a pointer to a wrapper object in the
native field instead of the file descriptor. All messsages about the
_NativeSocket sent to the EventHandler use the wrapper object instead of
the file descriptor. When the EventHandler closes the file, the file
descriptor in the wrapper object is set to -1 so that the finalizer will
instead do nothing.

On Windows, there is another level of indirection since the OS HANDLEs
were already wrapped in various kinds of Handle objects. As an additional
complication, ClientSocket close on Windows is asynchronous, so the
EventHandler may shutdown before all of the ClientSocket Handles can be
destroyed.

related #27898, #28081

R=johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/89dba57bcf6b5c6a28f9adee4b3f86195f64f6ae

[zra, 2017-03-25 22:39:50]

Self review

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:144: Retain();
// A reference to the Handle is Retained by the IO completion port.
// It is Released by DeleteIfClosed.

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:404: Retain();
// It is Released by DeleteIfClosed after ReadSyncCompleteAsync
// manually puts an event on the IO completion port.

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:563: handle->Release();
// Once the Handle is closed, no further events on the IO completion port
// will mention it. Thus, we can drop the reference here.

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:576: // Release the reference from the list.
// When an accept completes, we make a new ClientSocket (1 reference),
// and add it to the IO completion port (1 more reference). If an
// accepted connection is never requested by the Dart code, then
// this list owns a reference (first Release), and the IO completion
// port owns a reference, (second Release in DeleteIfClosed).

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:810: Retain();
// The write thread gets a reference to the Handle, which it places in
// the events it puts on the IO completion port. The reference is
// Released by DeleteIfClosed.

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/process_wi...
File runtime/bin/process_win.cc (right):

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/process_wi...
runtime/bin/process_win.cc:1010: write_handle->Release();
// Since SetConsoleCtrlHandler failed, the IO completion port will
// never receive an event for this handle, and will therefore never
// release the reference Retained by EnsureInitialized(). So, we
// have to do a second Release() here.

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/socket.cc
File runtime/bin/socket.cc (right):

https://codereview.chromium.org/2760293002/diff/180001/runtime/bin/socket.cc#...
runtime/bin/socket.cc:146: os_socket->socketfd->Retain();
// The same Socket is used by a second Dart _NativeSocket object.
// It Retains a reference.

[zra, 2017-03-26 22:36:11]

Description was changed from

==========
[dart:io] Adds a finalizer to _NativeSocket to avoid leaks
==========

to

==========
[dart:io] Adds a finalizer to _NativeSocket to avoid socket leaks

The finalizer sends the "close" message to the EventHandler for the
file descriptor in the _NativeSocket's native field. To avoid races and
spurious messages, this CL stores a pointer to a wrapper object in the
native field instead of the file descriptor. All messsages about the
_NativeSocket sent to the EventHandler use the wrapper object instead of
the file descriptor. When the EventHandler closes the file, the file
descriptor in the wrapper object is set to -1 so that the finalizer will
instead do nothing.

On Windows, there is another level of indirection since the OS HANDLEs
were already wrapped in various kinds of Handle objects. As an additional
complication, ClientSocket close on Windows is asynchronous, so the
EventHandler may shutdown before all of the ClientSocket Handles can be
destroyed.
==========

[zra, 2017-03-26 22:36:29]

zra@google.com changed reviewers:
+ asiva@google.com, johnmccutchan@google.com

[kevmoo, 2017-03-27 01:05:04]

kevmoo@google.com changed reviewers:
+ kevmoo@google.com

[kevmoo, 2017-03-27 01:05:05]

DBQ: does this fix a known issue?

There were a few hanging around that sound related – just curious.

[zra, 2017-03-27 02:23:15]

Description was changed from

==========
[dart:io] Adds a finalizer to _NativeSocket to avoid socket leaks

The finalizer sends the "close" message to the EventHandler for the
file descriptor in the _NativeSocket's native field. To avoid races and
spurious messages, this CL stores a pointer to a wrapper object in the
native field instead of the file descriptor. All messsages about the
_NativeSocket sent to the EventHandler use the wrapper object instead of
the file descriptor. When the EventHandler closes the file, the file
descriptor in the wrapper object is set to -1 so that the finalizer will
instead do nothing.

On Windows, there is another level of indirection since the OS HANDLEs
were already wrapped in various kinds of Handle objects. As an additional
complication, ClientSocket close on Windows is asynchronous, so the
EventHandler may shutdown before all of the ClientSocket Handles can be
destroyed.
==========

to

==========
[dart:io] Adds a finalizer to _NativeSocket to avoid socket leaks

The finalizer sends the "close" message to the EventHandler for the
file descriptor in the _NativeSocket's native field. To avoid races and
spurious messages, this CL stores a pointer to a wrapper object in the
native field instead of the file descriptor. All messsages about the
_NativeSocket sent to the EventHandler use the wrapper object instead of
the file descriptor. When the EventHandler closes the file, the file
descriptor in the wrapper object is set to -1 so that the finalizer will
instead do nothing.

On Windows, there is another level of indirection since the OS HANDLEs
were already wrapped in various kinds of Handle objects. As an additional
complication, ClientSocket close on Windows is asynchronous, so the
EventHandler may shutdown before all of the ClientSocket Handles can be
destroyed.

fixes #27898, #28081
==========

[zra, 2017-03-27 02:23:35]

On 2017/03/27 01:05:05, kevmoo wrote:
> DBQ: does this fix a known issue?
> 
> There were a few hanging around that sound related – just curious.

Update CL description with the issue numbers I could find.

[Cutch, 2017-03-27 17:17:50]

LGTM w/minor comments

https://codereview.chromium.org/2760293002/diff/200001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/2760293002/diff/200001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:594: AcceptEx_ = NULL;
Maybe a note explaining why you're NULLing the field.

https://codereview.chromium.org/2760293002/diff/200001/tests/standalone/io/so...
File tests/standalone/io/socket_bind_test.dart (right):

https://codereview.chromium.org/2760293002/diff/200001/tests/standalone/io/so...
tests/standalone/io/socket_bind_test.dart:113: await socket2.close();
maybe a comment why you're closing this socket.

[zra, 2017-03-27 19:50:14]

Description was changed from

==========
[dart:io] Adds a finalizer to _NativeSocket to avoid socket leaks

The finalizer sends the "close" message to the EventHandler for the
file descriptor in the _NativeSocket's native field. To avoid races and
spurious messages, this CL stores a pointer to a wrapper object in the
native field instead of the file descriptor. All messsages about the
_NativeSocket sent to the EventHandler use the wrapper object instead of
the file descriptor. When the EventHandler closes the file, the file
descriptor in the wrapper object is set to -1 so that the finalizer will
instead do nothing.

On Windows, there is another level of indirection since the OS HANDLEs
were already wrapped in various kinds of Handle objects. As an additional
complication, ClientSocket close on Windows is asynchronous, so the
EventHandler may shutdown before all of the ClientSocket Handles can be
destroyed.

fixes #27898, #28081
==========

to

==========
[dart:io] Adds a finalizer to _NativeSocket to avoid socket leaks

The finalizer sends the "close" message to the EventHandler for the
file descriptor in the _NativeSocket's native field. To avoid races and
spurious messages, this CL stores a pointer to a wrapper object in the
native field instead of the file descriptor. All messsages about the
_NativeSocket sent to the EventHandler use the wrapper object instead of
the file descriptor. When the EventHandler closes the file, the file
descriptor in the wrapper object is set to -1 so that the finalizer will
instead do nothing.

On Windows, there is another level of indirection since the OS HANDLEs
were already wrapped in various kinds of Handle objects. As an additional
complication, ClientSocket close on Windows is asynchronous, so the
EventHandler may shutdown before all of the ClientSocket Handles can be
destroyed.

related #27898, #28081
==========

[zra, 2017-03-28 14:40:19]

https://codereview.chromium.org/2760293002/diff/200001/runtime/bin/eventhandl...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/2760293002/diff/200001/runtime/bin/eventhandl...
runtime/bin/eventhandler_win.cc:594: AcceptEx_ = NULL;
On 2017/03/27 17:17:50, Cutch wrote:
> Maybe a note explaining why you're NULLing the field.

Done.

https://codereview.chromium.org/2760293002/diff/200001/tests/standalone/io/so...
File tests/standalone/io/socket_bind_test.dart (right):

https://codereview.chromium.org/2760293002/diff/200001/tests/standalone/io/so...
tests/standalone/io/socket_bind_test.dart:113: await socket2.close();
On 2017/03/27 17:17:50, Cutch wrote:
> maybe a comment why you're closing this socket.

Done.

[zra, 2017-03-28 14:44:10]

Description was changed from

==========
[dart:io] Adds a finalizer to _NativeSocket to avoid socket leaks

The finalizer sends the "close" message to the EventHandler for the
file descriptor in the _NativeSocket's native field. To avoid races and
spurious messages, this CL stores a pointer to a wrapper object in the
native field instead of the file descriptor. All messsages about the
_NativeSocket sent to the EventHandler use the wrapper object instead of
the file descriptor. When the EventHandler closes the file, the file
descriptor in the wrapper object is set to -1 so that the finalizer will
instead do nothing.

On Windows, there is another level of indirection since the OS HANDLEs
were already wrapped in various kinds of Handle objects. As an additional
complication, ClientSocket close on Windows is asynchronous, so the
EventHandler may shutdown before all of the ClientSocket Handles can be
destroyed.

related #27898, #28081
==========

to

==========
[dart:io] Adds a finalizer to _NativeSocket to avoid socket leaks

The finalizer sends the "close" message to the EventHandler for the
file descriptor in the _NativeSocket's native field. To avoid races and
spurious messages, this CL stores a pointer to a wrapper object in the
native field instead of the file descriptor. All messsages about the
_NativeSocket sent to the EventHandler use the wrapper object instead of
the file descriptor. When the EventHandler closes the file, the file
descriptor in the wrapper object is set to -1 so that the finalizer will
instead do nothing.

On Windows, there is another level of indirection since the OS HANDLEs
were already wrapped in various kinds of Handle objects. As an additional
complication, ClientSocket close on Windows is asynchronous, so the
EventHandler may shutdown before all of the ClientSocket Handles can be
destroyed.

related #27898, #28081

R=johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/89dba57bcf6b5c6a28f9adee4b3f86195f64f6ae
==========

[zra, 2017-03-28 14:44:11]

Committed patchset #12 (id:220001) manually as
89dba57bcf6b5c6a28f9adee4b3f86195f64f6ae (presubmit successful).