Always initialize the EventHandler in the standalone.

Always initialize the EventHandler in the standalone.

BUG=
R=sgjesse@google.com

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

[Søren Gjesse, 2013-08-21 13:52:04]

LGTM

https://codereview.chromium.org/22901017/diff/1/runtime/bin/eventhandler.cc
File runtime/bin/eventhandler.cc (right):

https://codereview.chromium.org/22901017/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:69: void EventHandler::Start() {
You don't need the mutex here any more.

https://codereview.chromium.org/22901017/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:70: MutexLocker locker(mutex_);
Assert that event_handler_ is NULL.

https://codereview.chromium.org/22901017/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:72: event_handler = new EventHandler();
If we want we can still postpone the actual start of the delegate until the
first message is actually send.

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

https://codereview.chromium.org/22901017/diff/1/runtime/bin/main.cc#newcode744
runtime/bin/main.cc:744: // Start eventhandler now, as it might need the
eventhandler to fully
Maybe just

// Start event handler.

[Anders Johnsen, 2013-08-22 11:31:05]

PTAL, Removed the Mutex in EventHandler. We now don't call EventHandler::Stop on
exit, as there may be child isolates still running. Once we are able to stop
them all, we can re-insert EventHandler::Stop.

https://codereview.chromium.org/22901017/diff/1/runtime/bin/eventhandler.cc
File runtime/bin/eventhandler.cc (right):

https://codereview.chromium.org/22901017/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:69: void EventHandler::Start() {
On 2013/08/21 13:52:05, Søren Gjesse wrote:
> You don't need the mutex here any more.

Done.

https://codereview.chromium.org/22901017/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:70: MutexLocker locker(mutex_);
On 2013/08/21 13:52:05, Søren Gjesse wrote:
> Assert that event_handler_ is NULL.

Done.

https://codereview.chromium.org/22901017/diff/1/runtime/bin/eventhandler.cc#n...
runtime/bin/eventhandler.cc:72: event_handler = new EventHandler();
On 2013/08/21 13:52:05, Søren Gjesse wrote:
> If we want we can still postpone the actual start of the delegate until the
> first message is actually send.

Yes, but I would like to avoid the extra check on each call. (it's not
expensive, but still...)

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

https://codereview.chromium.org/22901017/diff/1/runtime/bin/main.cc#newcode744
runtime/bin/main.cc:744: // Start eventhandler now, as it might need the
eventhandler to fully
On 2013/08/21 13:52:05, Søren Gjesse wrote:
> Maybe just
> 
> // Start event handler.

Done.

[Søren Gjesse, 2013-08-22 12:02:35]

LGTM!

[Anders Johnsen, 2013-08-22 12:04:21]

Committed patchset #2 manually as r26517 (presubmit successful).

[Ivan Posva, 2013-08-23 00:14:16]

I am a bit at a loss trying to figure out the motivation for this change.

Thanks,
-Ivan

https://codereview.chromium.org/22901017/diff/7001/runtime/bin/eventhandler_l...
File runtime/bin/eventhandler_linux.cc (right):

https://codereview.chromium.org/22901017/diff/7001/runtime/bin/eventhandler_l...
runtime/bin/eventhandler_linux.cc:170: // See: http://linux.die.net/man/7/pipe,
section 'Pipe_buf'.
You should probably ASSERT(kInterruptMessageSize < PIPE_BUF).

https://codereview.chromium.org/22901017/diff/7001/runtime/bin/eventhandler_m...
File runtime/bin/eventhandler_macos.cc (right):

https://codereview.chromium.org/22901017/diff/7001/runtime/bin/eventhandler_m...
runtime/bin/eventhandler_macos.cc:189: // See: http://linux.die.net/man/7/pipe,
section 'Pipe_buf'.
It is a bit questionable quoting a Linux man page for a Mac OS X implementation.

[Anders Johnsen, 2013-08-23 05:15:27]

Hi Ivan,

Sorry for the lack of motivation about this. It's a two things I'm trying to
resolve:

One is to have a guard variable in Dart that we check for each SendData call.
There is no reason Dart should be the place where we put in such a guard, the
runtime is much better. So instead of just transferring the guard to the C++
runtime, I've added so that we always start it (it's the server-side program
after-all).

Second is we have a Mutex we lock every single time we call SendData. The only
reason (due the fact that writing to pipe/completion-ports are atomic in our
case) is that we might close the event-handler before child isolates are
terminated. The simple (temporary) solution is to remove the Mutex and just not
call EventHandler::Stop(). We can change that once we have a way to kill child
isolates first.

I've addressed your comments in https://codereview.chromium.org/22901017/.

Cheers,

- Anders

https://codereview.chromium.org/22901017/diff/7001/runtime/bin/eventhandler_l...
File runtime/bin/eventhandler_linux.cc (right):

https://codereview.chromium.org/22901017/diff/7001/runtime/bin/eventhandler_l...
runtime/bin/eventhandler_linux.cc:170: // See: http://linux.die.net/man/7/pipe,
section 'Pipe_buf'.
On 2013/08/23 00:14:16, Ivan Posva wrote:
> You should probably ASSERT(kInterruptMessageSize < PIPE_BUF).

Good idea.

https://codereview.chromium.org/22901017/diff/7001/runtime/bin/eventhandler_m...
File runtime/bin/eventhandler_macos.cc (right):

https://codereview.chromium.org/22901017/diff/7001/runtime/bin/eventhandler_m...
runtime/bin/eventhandler_macos.cc:189: // See: http://linux.die.net/man/7/pipe,
section 'Pipe_buf'.
On 2013/08/23 00:14:16, Ivan Posva wrote:
> It is a bit questionable quoting a Linux man page for a Mac OS X
implementation.

You are right. I was not able to find it in a mac os x man page, but I did find
it in some books when searching. I'm adding the assert and removing the link.

[Anders Johnsen, 2013-08-23 05:16:26]

I meant https://codereview.chromium.org/22918023 :)