Wait for eventhandler to shut down before exiting

Wait for eventhandler to shut down before exiting

R=sgjesse@google.com

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

[kustermann, 2015-02-04 20:58:43]

kustermann@google.com changed reviewers:
+ iposva@google.com, sgjesse@google.com

[kustermann, 2015-02-04 20:58:43]

@sgjesse:
This race between 
- exit() in the main thread [which invokes destructors of global/static objects]
- and the event handler still talking to the tcp listening registry
caused one of the buildbot issues (though there are others) if I'm right.

@Ivan/Soeren:
Committing this CL might reveal more race conditions when shutting the VM down.
I hope we can take a look at this soon.

https://codereview.chromium.org/875403006/diff/1/dart/runtime/bin/eventhandle...
File dart/runtime/bin/eventhandler_android.cc (left):

https://codereview.chromium.org/875403006/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler_android.cc:303:
reinterpret_cast<EventHandlerImplementation*>(args);
This was a bad cast, it just happens by accident that

handler == &handler->delegate_ 

because delegate_ is the first member in the class.

https://codereview.chromium.org/875403006/diff/1/dart/runtime/bin/main.cc
File dart/runtime/bin/main.cc (right):

https://codereview.chromium.org/875403006/diff/1/dart/runtime/bin/main.cc#new...
dart/runtime/bin/main.cc:1102: EventHandler::Stop();
I'm actually not 100% sure whether this should be here or before
Dart_ShutdownIsolate().

What can happen is that the event handler thread is still posting messages to a
`Dart_Port` as long as EventHandler::Stop() isn't done.

[What happens if the eventhandler posts a message to a Dart_Port where the
receive port is in an isolate that is already dead?]

[Søren Gjesse, 2015-02-04 21:57:09]

lgtm

https://codereview.chromium.org/875403006/diff/1/dart/runtime/bin/eventhandler.h
File dart/runtime/bin/eventhandler.h (right):

https://codereview.chromium.org/875403006/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:123: * Signal to main thread that event handler
is done.
Maybe add "This is called by the event handler thread to ..."

https://codereview.chromium.org/875403006/diff/1/dart/runtime/bin/eventhandle...
dart/runtime/bin/eventhandler.h:133: * Stop the event-handler. It's expected
that there will be no further calls
Maybe add "This is called by the main thread to ..."

https://codereview.chromium.org/875403006/diff/1/dart/runtime/bin/main.cc
File dart/runtime/bin/main.cc (right):

https://codereview.chromium.org/875403006/diff/1/dart/runtime/bin/main.cc#new...
dart/runtime/bin/main.cc:1102: EventHandler::Stop();
On 2015/02/04 20:58:43, kustermann wrote:
> I'm actually not 100% sure whether this should be here or before
> Dart_ShutdownIsolate().
> 
> What can happen is that the event handler thread is still posting messages to
a
> `Dart_Port` as long as EventHandler::Stop() isn't done.
> 
> [What happens if the eventhandler posts a message to a Dart_Port where the
> receive port is in an isolate that is already dead?]

It should be safe to post to send ports where the the receive port is gone. That
should have no effect.

[kustermann, 2015-02-05 12:14:01]

New patchsets have been uploaded after l-g-t-m from sgjesse@google.com

[kustermann, 2015-02-05 12:15:58]

I removed now the call to stop the eventhandler when cleaning up, since the VM
doesn't support clean shutdowns yet.


This makes this CL commit (or rather modify) dead code. In case the VM will at
some point shut down cleanly, at least the race condition with
Eventhandler::Stop() would be gone if I commit this CL.

Up to you whether I should drop it.

[Søren Gjesse, 2015-02-05 12:35:38]

On 2015/02/05 12:15:58, kustermann wrote:
> I removed now the call to stop the eventhandler when cleaning up, since the VM
> doesn't support clean shutdowns yet.
> 
> 
> This makes this CL commit (or rather modify) dead code. In case the VM will at
> some point shut down cleanly, at least the race condition with
> Eventhandler::Stop() would be gone if I commit this CL.
> 
> Up to you whether I should drop it.

LGTM, let's commit it

[kustermann, 2015-02-05 13:16:33]

Committed patchset #2 (id:20001) manually as 43504.