Keep track of when a socket has been destroyed

Keep track of when a socket has been destroyed

This adds the event CLOSED to RawSocket and RawServerSocket. This
event indicate that the underlying socket is now destroyed.

This is used to make Socket.close and ServerSocket.close return a
future which is completed when the unserlying socket is
destroyed. This can be used to start listening on the same port after
closing a server socket.

R=ajohnsen@google.com, kustermann@google.com

BUG=https://code.google.com/p/dart/issues/detail?id=4155

Committed: https://code.google.com/p/dart/source/detail?r=25213

[Anders Johnsen, 2013-05-08 13:11:47]

LGTM!

https://codereview.chromium.org/14864009/diff/1/runtime/bin/socket_patch.dart
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/14864009/diff/1/runtime/bin/socket_patch.dart...
runtime/bin/socket_patch.dart:975: void destroy() {
return Future<Socket>.

https://codereview.chromium.org/14864009/diff/1/tests/standalone/io/echo_serv...
File tests/standalone/io/echo_server_stream_test.dart (right):

https://codereview.chromium.org/14864009/diff/1/tests/standalone/io/echo_serv...
tests/standalone/io/echo_server_stream_test.dart:44: print("onData");
Debug stuff in this file.

https://codereview.chromium.org/14864009/diff/1/tests/standalone/io/http_10_t...
File tests/standalone/io/http_10_test.dart (right):

https://codereview.chromium.org/14864009/diff/1/tests/standalone/io/http_10_t...
tests/standalone/io/http_10_test.dart:33: //Expect.throws(() =>
response.write("x"),
Debug stuff in this file.

[Anders Johnsen, 2013-06-27 14:26:39]

First update on this after working on it a bit.

DisconnectEx can be compared to shutdown(write), meaning that it'll hang until
the socket sees a close_read. The current implementation tries to substitute
disconnectex with closesocket. That won't work.

I'm not entirely sure on how to proceed, I'll look at this again later on.

[Anders Johnsen, 2013-07-18 10:38:44]

Martin, can you take a look at this? Let me know if you need more info for
reviewing this!

- Anders

[kustermann, 2013-07-18 11:34:46]

The main thing I don't understand is eventhandler_win.cc

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler.h
File runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler....
runtime/bin/eventhandler.h:17: // information see the comments in
socket_impl.dart
Is this comment out of date. Seems like these constants need to be kept in sync
with socket_patch.dart?

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_android.cc:211: ASSERT(msg.data == (1 <<
kCloseCommand));
This assert will be hit, if  'msg.data == kCloseCommand | kDestroyedEvent'!
(see comment in other file)

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:640: if (p != ILLEGAL_PORT)
DartUtils::PostInt32(p, 1 << kDestroyedEvent);
If you closed a socket with 'DisconnectEx_' then you should send back the
destroy event. Why do you have this condition here?

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:762: handle->Close();
I thought 'handle->Close()' will result in 'IssueDisconnect()'.

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:771: DartUtils::PostInt32(port, 1 <<
kDestroyedEvent);
Why do you do it here again?

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:791: DartUtils::PostInt32(port, 1 <<
kDestroyedEvent);
I don't understand why these PostInt32(kDestroyEvent) are all over the place
here.

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/socket_patch....
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/socket_patch....
runtime/bin/socket_patch.dart:458: isClosedRead = true;
As far as I understand it, isClosedRead indicates if we got an EOF.
So Why is this change necessary?

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/socket_patch....
runtime/bin/socket_patch.dart:535: sendToEventHandler(1 << CLOSE_COMMAND | 1 <<
DESTROYED_EVENT);
I thought that we send the CLOSE_COMMAND to the event handler and it will
respond with a DESTROYED_EVENT.

Why do you add the DESTROYED_EVENT here?

https://codereview.chromium.org/14864009/diff/27001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/14864009/diff/27001/sdk/lib/io/http_impl.dart...
sdk/lib/io/http_impl.dart:1935: result = new Future.value();
If somebody calls close() twice and once the future would complete with an
error, then the second time he should get the same future (which should complete
with the same error).

Is it guaranteed that we never get into an error situation?

https://codereview.chromium.org/14864009/diff/27001/tests/standalone/io/raw_s...
File tests/standalone/io/raw_socket_test.dart (right):

https://codereview.chromium.org/14864009/diff/27001/tests/standalone/io/raw_s...
tests/standalone/io/raw_socket_test.dart:331:
Expect.isFalse(closedEventReceived);
Indentation

[Anders Johnsen, 2013-07-19 10:49:07]

PTAL

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler.h
File runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler....
runtime/bin/eventhandler.h:17: // information see the comments in
socket_impl.dart
On 2013/07/18 11:34:46, kustermann wrote:
> Is this comment out of date. Seems like these constants need to be kept in
sync
> with socket_patch.dart?

They are in sync?

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_android.cc:211: ASSERT(msg.data == (1 <<
kCloseCommand));
On 2013/07/18 11:34:46, kustermann wrote:
> This assert will be hit, if  'msg.data == kCloseCommand | kDestroyedEvent'!
> (see comment in other file)

Removed in other file.

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:640: if (p != ILLEGAL_PORT)
DartUtils::PostInt32(p, 1 << kDestroyedEvent);
On 2013/07/18 11:34:46, kustermann wrote:
> If you closed a socket with 'DisconnectEx_' then you should send back the
> destroy event. Why do you have this condition here?

In Socket::Connect we can end up creating a socket and then deleting it, before
we get it back in to Dart, thus not having a Dart port associated.

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:762: handle->Close();
On 2013/07/18 11:34:46, kustermann wrote:
> I thought 'handle->Close()' will result in 'IssueDisconnect()'.

It will, but this is for more kinds than just ClientSocket.

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:771: DartUtils::PostInt32(port, 1 <<
kDestroyedEvent);
On 2013/07/18 11:34:46, kustermann wrote:
> Why do you do it here again?

ClientSocket will never be delete any other place than in DisconnectComplete.

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:791: DartUtils::PostInt32(port, 1 <<
kDestroyedEvent);
On 2013/07/18 11:34:46, kustermann wrote:
> I don't understand why these PostInt32(kDestroyEvent) are all over the place
> here.

Every time we delete a socket, we send a destroy event (with ClientSocket being
an 'exception' to some degree.

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/socket_patch....
File runtime/bin/socket_patch.dart (right):

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/socket_patch....
runtime/bin/socket_patch.dart:458: isClosedRead = true;
On 2013/07/18 11:34:46, kustermann wrote:
> As far as I understand it, isClosedRead indicates if we got an EOF.
> So Why is this change necessary?

Since we don't want to modify the state after the Socket is being shut down.

https://codereview.chromium.org/14864009/diff/27001/runtime/bin/socket_patch....
runtime/bin/socket_patch.dart:535: sendToEventHandler(1 << CLOSE_COMMAND | 1 <<
DESTROYED_EVENT);
On 2013/07/18 11:34:46, kustermann wrote:
> I thought that we send the CLOSE_COMMAND to the event handler and it will
> respond with a DESTROYED_EVENT.
> 
> Why do you add the DESTROYED_EVENT here?

Yes, this is very odd. Removed.

https://codereview.chromium.org/14864009/diff/27001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/14864009/diff/27001/sdk/lib/io/http_impl.dart...
sdk/lib/io/http_impl.dart:1935: result = new Future.value();
On 2013/07/18 11:34:46, kustermann wrote:
> If somebody calls close() twice and once the future would complete with an
> error, then the second time he should get the same future (which should
complete
> with the same error).
> 
> Is it guaranteed that we never get into an error situation?

Yes, it'll never complete with an error.

https://codereview.chromium.org/14864009/diff/27001/tests/standalone/io/raw_s...
File tests/standalone/io/raw_socket_test.dart (right):

https://codereview.chromium.org/14864009/diff/27001/tests/standalone/io/raw_s...
tests/standalone/io/raw_socket_test.dart:331:
Expect.isFalse(closedEventReceived);
On 2013/07/18 11:34:46, kustermann wrote:
> Indentation

Done.

[kustermann, 2013-07-19 11:59:18]

lgtm :-)

(But since I'm not the best reviewer for dart:io, you might want to wait for
soeren/bill [or you're confident enough that it doesn't contain any bugs].)

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:688: bool delete_handle = false;
Remove that boolean.

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:717: }
remove this.

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:773: delete_handle = true;
remove this.

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:778: DeleteIfClosed(handle);
Remove the condition.

[Anders Johnsen, 2013-07-19 13:25:58]

Thanks Martin. I'll try and land, while being very eager to revert :)

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_win.cc (right):

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:688: bool delete_handle = false;
On 2013/07/19 11:59:18, kustermann wrote:
> Remove that boolean.

Done.

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:717: }
On 2013/07/19 11:59:18, kustermann wrote:
> remove this.

Done.

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:773: delete_handle = true;
On 2013/07/19 11:59:18, kustermann wrote:
> remove this.

Done.

https://codereview.chromium.org/14864009/diff/41001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_win.cc:778: DeleteIfClosed(handle);
On 2013/07/19 11:59:18, kustermann wrote:
> Remove the condition.

Done.

[Anders Johnsen, 2013-07-19 13:27:00]

Committed patchset #13 manually as r25213 (presubmit successful).