Allow embedders to provide custom message delivery for an isolate.

Allow embedders to provide custom message delivery for an isolate.

==============

Added Dart_SetPostMessageCallback and Dart_SetClosePortCallback.
These allow the embedder to provide custom message/port behavior
for their application. The vm provides standard implementations
that work with the standard run loop.

Added Dart_HandleMessage, which processes one message on the
current isolate. Embedders can use this to write their own
message processing loops. Rewrote code to use this internally.

Added Isolate::StandardRunLoop() to share code between
Dart_RunLoop and lib/isolate.cc

Changed the interface to PortMap::PostMessage. PostMessage is
now agnostic to message delivery mechanism. Note that PortMap is
now out of the "ReceiveMessage" business entirely. Moved
MessageQueue and friends out to message_queue.cc/h.

Moved the monitor from the Isolate into the MessageQueue. No
need for outsiders to mess. Added MessageQueue::Wait. Moved
monitor locking from PortMap into MessageQueue itself, which was
easier for me to reason about. Wrote some tests.

Removed PortMessage::Handle. The code turned into Dart_HandleMessage.

Regularized the nomenclature around ports. Type is now always
Dart_Port instead of intptr_t. Variables end in _port instead of
_id. Use the term "dest" instead of "target" or "send".

Added a family of new tests to port_test.

Added EXPECT_NE to the test framework.
Committed: https://code.google.com/p/dart/source/detail?r=516

[turnidge, 2011-10-14 18:44:01]

Okay, here's the review on the correct branch...

[siva, 2011-10-14 21:01:52]

LGTM with some comments.

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:85: // and when spawning new isolates from Dart code.
 The result returned
extra space between . and The

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:197: DART_EXPORT Dart_Result Dart_RunLoop();
I am wondering if we should add a comment to this function stating that this is
a convenience method provided for processing inter-isolate messages and can be
used only if the embedder has not provided any alternate message delivery
mechanism.

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:203: // must eventually cause Dart_HandleMessage to
be called to process
"callback is must eventually" seems incorrect.

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:223: // settign only applies to the current isolate.
*settign*setting

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:226: // Directly handle a message on the current
isolate.
"Handle a message.."? not sure what Directly handle implies.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc#newcode7
runtime/vm/isolate.cc:7: #include "include/dart_api.h"
The style has been to leave a blank line here (i.e when the directory name
changes).

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc#newcode25
runtime/vm/isolate.cc:25: 
extra blank line here.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc#newcode248
runtime/vm/isolate.cc:248: }
Why not make the Dequeue method wait if there is no message instead of having it
return null and then having to explicitly call wait again.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc#newcode252
runtime/vm/isolate.cc:252: // message->Handle();
Is this commented out code still necessary?

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue.cc#new...
runtime/vm/message_queue.cc:34: monitor_.Notify();
Is it necessary to notify always? maybe ok to notify only if the queue was
empty.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue.cc#new...
runtime/vm/message_queue.cc:43: }
Same question should we have the Dequeue wait instead?
Getting a message out from an empty queue involves taking the lock 3 times.

In the old code since the lock was outside, we could take the lock once and do
all the operations (check, wait and get) in one block.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.cc
File runtime/vm/message_queue_test.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:74: queue->Wait(0);  // Waits on internal
monitor
monitor.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:96: ml.Wait(5);
Instead of a timed wait why not share the waiter monitor
and have the MessageReceiver_start thread notify it. That way we won't have
delays in case this thread executes earlier than the other one.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:106: ml.Wait(i+1);
Why do you need this wait here?

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:144: queue.Flush(port1);
Would be good to have two additional tests
- a port with 0 messages
- a port with more than 1 message.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:154: // msg1 is already deleted by Flush
Flush.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port.cc
File runtime/vm/port.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port.cc#newcode157
runtime/vm/port.cc:157: ASSERT(callback);
ASSERT(port != kCloseAllPorts);

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc
File runtime/vm/port_test.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc#newcode66
runtime/vm/port_test.cc:66: // Uniqueness
'.'

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc#newcode88
runtime/vm/port_test.cc:88: // Close all ports at once
once.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc#newcode127
runtime/vm/port_test.cc:127: EXPECT_STREQ("", saved_msg);
The duped string is leaked in this case.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc#newcode164
runtime/vm/port_test.cc:164: // all messages in the message queue are for active
ports.
"that the all messages ..." => "that all messages ..."

[turnidge, 2011-10-14 23:08:02]

Please take another look.

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:85: // and when spawning new isolates from Dart code.
 The result returned
On 2011/10/14 21:01:52, asiva wrote:
> extra space between . and The

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:197: DART_EXPORT Dart_Result Dart_RunLoop();
On 2011/10/14 21:01:52, asiva wrote:
> I am wondering if we should add a comment to this function stating that this
is
> a convenience method provided for processing inter-isolate messages and can be
> used only if the embedder has not provided any alternate message delivery
> mechanism.

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:203: // must eventually cause Dart_HandleMessage to
be called to process
On 2011/10/14 21:01:52, asiva wrote:
> "callback is must eventually" seems incorrect.

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:223: // settign only applies to the current isolate.
On 2011/10/14 21:01:52, asiva wrote:
> *settign*setting

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/include/dart_api.h#newc...
runtime/include/dart_api.h:226: // Directly handle a message on the current
isolate.
On 2011/10/14 21:01:52, asiva wrote:
> "Handle a message.."? not sure what Directly handle implies.

"Directly" meant "immediately", as opposed to queuing it up for later or
something.  I've just dropped the word.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc#newcode7
runtime/vm/isolate.cc:7: #include "include/dart_api.h"
On 2011/10/14 21:01:52, asiva wrote:
> The style has been to leave a blank line here (i.e when the directory name
> changes).

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc#newcode25
runtime/vm/isolate.cc:25: 
On 2011/10/14 21:01:52, asiva wrote:
> extra blank line here.

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc#newcode248
runtime/vm/isolate.cc:248: }
On 2011/10/14 21:01:52, asiva wrote:
> Why not make the Dequeue method wait if there is no message instead of having
it
> return null and then having to explicitly call wait again.

Done.  Thanks, that's better.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/isolate.cc#newcode252
runtime/vm/isolate.cc:252: // message->Handle();
On 2011/10/14 21:01:52, asiva wrote:
> Is this commented out code still necessary?

Removed.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue.cc#new...
runtime/vm/message_queue.cc:34: monitor_.Notify();
On 2011/10/14 21:01:52, asiva wrote:
> Is it necessary to notify always? maybe ok to notify only if the queue was
> empty.

Changed.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue.cc#new...
runtime/vm/message_queue.cc:43: }
Yes, I agree.  Thanks.

On 2011/10/14 21:01:52, asiva wrote:
> Same question should we have the Dequeue wait instead?
> Getting a message out from an empty queue involves taking the lock 3 times.
> 
> In the old code since the lock was outside, we could take the lock once and do
> all the operations (check, wait and get) in one block.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.cc
File runtime/vm/message_queue_test.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:74: queue->Wait(0);  // Waits on internal
monitor
On 2011/10/14 21:01:52, asiva wrote:
> monitor.

Line removed.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:96: ml.Wait(5);
On 2011/10/14 21:01:52, asiva wrote:
> Instead of a timed wait why not share the waiter monitor
> and have the MessageReceiver_start thread notify it. That way we won't have
> delays in case this thread executes earlier than the other one.

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:106: ml.Wait(i+1);
On 2011/10/14 21:01:52, asiva wrote:
> Why do you need this wait here?

I wanted to exercise the path where the monitor is empty and we actually block
on the queue.  I have restructured the test to have 3 messages delivered "right
away" and 3 more messages show up later.  This should exercise both the blocking
and non-blocking code paths most of the time.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:144: queue.Flush(port1);
On 2011/10/14 21:01:52, asiva wrote:
> Would be good to have two additional tests
> - a port with 0 messages
> - a port with more than 1 message.

I added two test cases, but I'm not sure that they are what you actually had in
mind.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/message_queue_test.c...
runtime/vm/message_queue_test.cc:154: // msg1 is already deleted by Flush
On 2011/10/14 21:01:52, asiva wrote:
> Flush.

Fixed.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port.cc
File runtime/vm/port.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port.cc#newcode157
runtime/vm/port.cc:157: ASSERT(callback);
On 2011/10/14 21:01:52, asiva wrote:
> ASSERT(port != kCloseAllPorts);

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc
File runtime/vm/port_test.cc (right):

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc#newcode66
runtime/vm/port_test.cc:66: // Uniqueness
On 2011/10/14 21:01:52, asiva wrote:
> '.'

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc#newcode88
runtime/vm/port_test.cc:88: // Close all ports at once
On 2011/10/14 21:01:52, asiva wrote:
> once.

Done.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc#newcode127
runtime/vm/port_test.cc:127: EXPECT_STREQ("", saved_msg);
On 2011/10/14 21:01:52, asiva wrote:
> The duped string is leaked in this case.

Ah.  Good catch.  I have added a call to free() in PostMessage and updated the
comments for PostMessage and for the message post callback in dart_api.h to be
explicit about the memory management.

http://codereview.chromium.org/8297004/diff/1/runtime/vm/port_test.cc#newcode164
runtime/vm/port_test.cc:164: // all messages in the message queue are for active
ports.
On 2011/10/14 21:01:52, asiva wrote:
> "that the all messages ..." => "that all messages ..."

I have just deleted this comment.

[siva, 2011-10-14 23:50:32]

LGTM

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:197: // A convenience routine for which processes any
incoming messages for
routine which

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:199: // current isolate are closed
closed.

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:156: ProcessUnhandledException(uhe);
Not for this changelist but in a future changelist we should consider just
returning the exception from this method so that the embedder can choose to
handle the error anyway it wants instead of exiting.
In Isolate::StandardRunLoop we could check the return value from
Dart_HandleMessage and call ProcessUnhandledException.

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue.cc#...
runtime/vm/message_queue.cc:18: // isolate.  Should MonitorLocker really be a
StackResource?
Is this comment still valid? Since the code in this block is not doing any
LongJumps from here does it matter if MonitorLocker is a StackResource or not.

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue.cc#...
runtime/vm/message_queue.cc:44: monitor_.Wait(millis);
ml.Wait(...);

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue_tes...
File runtime/vm/message_queue_test.cc (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue_tes...
runtime/vm/message_queue_test.cc:220: }
I was thinking of having a port3 with 0 messages and
port4 with 2 messages and invoking queue.Flush(port3) and queue.Flush(port4) in
MessageQueue_Flush itself.

This is fine too having explicit tests for these cases.

[Anton Muhin, 2011-10-17 15:20:29]

Sorry for late response, was OOO.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:208: // delivery mechanism for inter-isolate
messages. The callback must
is it indeed a must?  May I just drop some messages?

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:220: // Allow embedders to provide an alternate
delivery mechanism for
nit: it looks like 3rd person is preferred: allow[s], etc.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:223: DART_EXPORT void
Dart_SetPostMessageCallback(Dart_PostMessageCallback callback);
is _SetPostMessageCallback thread safe?  Imagine message being  sent while I am
changing the callback.  That's probably should as I have no way to control this,
but that probably should be explicit.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:226: // when a port is closed. The constant
'kCloseAllPorts' is passed
nit: in port parameter?

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:237: DART_EXPORT void Dart_HandleMessage(Dart_Port
dest_port,
do we need to pass ports around?  maybe we can bundle them into a message (at
least at API level)?

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1564: bool result = PortMap::PostMessage(port, 0,
buffer);
0 -> kCloseAll?

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/isolate.h#newcode52
runtime/vm/isolate.h:52: void set_post_message_callback(Dart_PostMessageCallback
value) {
no synchronization, might be a problem.

[turnidge, 2011-10-17 18:38:48]

PTAL

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:197: // A convenience routine for which processes any
incoming messages for
On 2011/10/14 23:50:32, asiva wrote:
> routine which

Done.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:199: // current isolate are closed
On 2011/10/14 23:50:32, asiva wrote:
> closed.

Done.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:208: // delivery mechanism for inter-isolate
messages. The callback must
On 2011/10/17 15:20:29, antonm1 wrote:
> is it indeed a must?  May I just drop some messages?

I suppose it isn't a must, if you don't care about getting the messages.  I have
rephrased it...

// A post message callback allows the embedder to provide an alternate
// delivery mechanism for inter-isolate messages. It is the
// responsibility of the embedder to call Dart_HandleMessage to
// process the message.

If this is still too prescriptive, let me know.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:220: // Allow embedders to provide an alternate
delivery mechanism for
On 2011/10/17 15:20:29, antonm1 wrote:
> nit: it looks like 3rd person is preferred: allow[s], etc.

Done.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:223: DART_EXPORT void
Dart_SetPostMessageCallback(Dart_PostMessageCallback callback);
On 2011/10/17 15:20:29, antonm1 wrote:
> is _SetPostMessageCallback thread safe?  Imagine message being  sent while I
am
> changing the callback.  That's probably should as I have no way to control
this,
> but that probably should be explicit.

Added some documentation around this.

I also combined SetPostMessageCallback and SetClosePortCallback to show the user
that they should be overridden together.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:226: // when a port is closed. The constant
'kCloseAllPorts' is passed
On 2011/10/17 15:20:29, antonm1 wrote:
> nit: in port parameter?

Done.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:237: DART_EXPORT void Dart_HandleMessage(Dart_Port
dest_port,
On 2011/10/17 15:20:29, antonm1 wrote:
> do we need to pass ports around?  maybe we can bundle them into a message (at
> least at API level)?

They are sort of useless for the default, standalone embedding.  But once we
start needing to communicate with isolates in other processes and on other
machines, then the embedder will need access to the port number (which is more
of a local entity) so that they can map to the correct non-local isolate/port.

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:156: ProcessUnhandledException(uhe);
On 2011/10/14 23:50:32, asiva wrote:
> Not for this changelist but in a future changelist we should consider just
> returning the exception from this method so that the embedder can choose to
> handle the error anyway it wants instead of exiting.
> In Isolate::StandardRunLoop we could check the return value from
> Dart_HandleMessage and call ProcessUnhandledException.

Added TODO

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1564: bool result = PortMap::PostMessage(port, 0,
buffer);
On 2011/10/17 15:20:29, antonm1 wrote:
> 0 -> kCloseAll?

I have added a kNoReplyPort constant which means there is no reply port. 
Confusingly, it is the same value as kCloseAllPorts.  I was hunting for a name
that would work for both, but "kAllOrNothingPort" is pretty confusing :-).

Maybe kNoSuchPort... let me know what you think...

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/isolate.h#newcode52
runtime/vm/isolate.h:52: void set_post_message_callback(Dart_PostMessageCallback
value) {
On 2011/10/17 15:20:29, antonm1 wrote:
> no synchronization, might be a problem.

I've added a comment for now.

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue.cc#...
runtime/vm/message_queue.cc:18: // isolate.  Should MonitorLocker really be a
StackResource?
On 2011/10/14 23:50:32, asiva wrote:
> Is this comment still valid? Since the code in this block is not doing any
> LongJumps from here does it matter if MonitorLocker is a StackResource or not.

There is an assertion for all StrackResources (including MonitorLocker) that
there is a current isolate.  When I run the tests, I have seen this trigger.  I
could retry this now, but my impression would be that I would still see this
assertion fire during unit testing.  Notice that PortMap::PostMessage avoids
using MutexLocker fo the same reason.

I can retry this again if you want.

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue.cc#...
runtime/vm/message_queue.cc:44: monitor_.Wait(millis);
On 2011/10/14 23:50:32, asiva wrote:
> ml.Wait(...);

Done.

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue_tes...
File runtime/vm/message_queue_test.cc (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/vm/message_queue_tes...
runtime/vm/message_queue_test.cc:220: }
On 2011/10/14 23:50:32, asiva wrote:
> I was thinking of having a port3 with 0 messages and
> port4 with 2 messages and invoking queue.Flush(port3) and queue.Flush(port4)
in
> MessageQueue_Flush itself.
> 
> This is fine too having explicit tests for these cases.

Ok.  I like to keep my unit tests as small as I can so I will leave this as is.

[Anton Muhin, 2011-10-18 07:01:03]

Todd, it looks like you haven't uploaded a new version of patch.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:197: // A convenience routine for which processes any
incoming messages for
I think Siva meant "A convenience routine which processes...", w/o for

On 2011/10/17 18:38:48, turnidge wrote:
> On 2011/10/14 23:50:32, asiva wrote:
> > routine which
> 
> Done.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:199: // current isolate are closed
Nit, missing trailing dot.

On 2011/10/17 18:38:48, turnidge wrote:
> On 2011/10/14 23:50:32, asiva wrote:
> > closed.
> 
> Done.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:208: // delivery mechanism for inter-isolate
messages. The callback must
I like your new wording, thanks a lot.

On 2011/10/17 18:38:48, turnidge wrote:
> On 2011/10/17 15:20:29, antonm1 wrote:
> > is it indeed a must?  May I just drop some messages?
> 
> I suppose it isn't a must, if you don't care about getting the messages.  I
have
> rephrased it...
> 
> // A post message callback allows the embedder to provide an alternate
> // delivery mechanism for inter-isolate messages. It is the
> // responsibility of the embedder to call Dart_HandleMessage to
> // process the message.
> 
> If this is still too prescriptive, let me know.

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:213: // is the responsibility of the callback to
ensure that free(message)
that somewhat leaks the abstraction---we have this alias for Dart_Message.  You
may want to add Dart_ReleaseMessage(Dart_Message), but feel free to ignore if it
smells like overengineering.

[turnidge, 2011-10-18 16:42:14]

Sorry, forgot to upload.  Uploading now...

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:197: // A convenience routine for which processes any
incoming messages for
Yeah, forgot to upload, sorry.

On 2011/10/18 07:01:03, antonmuhin wrote:
> I think Siva meant "A convenience routine which processes...", w/o for
> 
> On 2011/10/17 18:38:48, turnidge wrote:
> > On 2011/10/14 23:50:32, asiva wrote:
> > > routine which
> > 
> > Done.
>

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:199: // current isolate are closed
Ditto upload.

On 2011/10/18 07:01:03, antonmuhin wrote:
> Nit, missing trailing dot.
> 
> On 2011/10/17 18:38:48, turnidge wrote:
> > On 2011/10/14 23:50:32, asiva wrote:
> > > closed.
> > 
> > Done.
>

http://codereview.chromium.org/8297004/diff/7001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:213: // is the responsibility of the callback to
ensure that free(message)
On 2011/10/18 07:01:03, antonmuhin wrote:
> that somewhat leaks the abstraction---we have this alias for Dart_Message. 
You
> may want to add Dart_ReleaseMessage(Dart_Message), but feel free to ignore if
it
> smells like overengineering.

I agree that this leaks the abstraction.  I've added a TODO to address it later.
 We will need to add additional message processing functions for those who will
need to send the message over the wire.  I will address it then.

[Anton Muhin, 2011-10-18 17:19:58]

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:205: // Messages/ports
nit: missing trailing dot I believe.

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:207: // A post message callback allows the embedder
to provide an alternate
nit: alternate -> alternative ?

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:213: // passed as the reply_port parameter.
nit: reply_port -> 'reply_port' ?

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:216: // is the responsibility of the callback to
ensure that free(message)
hmm, you're saying "Claims ownership of the memory pointed to by 'message' and
will ensure that free(message) is called." in PortMap::PostMessage.

Contradiction?

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:220: typedef bool
(*Dart_PostMessageCallback)(Dart_Isolate dest_isolate,
what's the semantics of returned value?

http://codereview.chromium.org/8297004/diff/15001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/lib/isolate.cc#newc...
runtime/lib/isolate.cc:206: // Keep listening until there are no active receive
ports.
nit: this comment can probably go away now.

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:104: Isolate* isolate= Isolate::Current();
nit: space before =

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:152: // exception so taht eh embedder can choose how
to handle this
nit: so [that] [the] embedder can choose

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:167: // Keep listening until there are no active
receive ports.
nit: ditto for comment.

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/isolate.h#newcode49
runtime/vm/isolate.h:49: Dart_PostMessageCallback post_message_callback() const
{
will you add a TODO to handle concurrent update of isolate's callbacks and
message sending?

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/message_queue.cc...
runtime/vm/message_queue.cc:43: if (result == NULL) {
should it be if or while?  Wait is usually in the loop, but you might have
stronger guarantees (but I would keep it in the loop anyway).

[turnidge, 2011-10-18 17:33:40]

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:205: // Messages/ports
On 2011/10/18 17:19:58, antonmuhin wrote:
> nit: missing trailing dot I believe.

This comment is intended as a title for the section.  It isn't a sentence so it
feels wrong to end it with a period.

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:207: // A post message callback allows the embedder
to provide an alternate
On 2011/10/18 17:19:58, antonmuhin wrote:
> nit: alternate -> alternative ?

Done.

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:213: // passed as the reply_port parameter.
On 2011/10/18 17:19:58, antonmuhin wrote:
> nit: reply_port -> 'reply_port' ?

Done.

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:216: // is the responsibility of the callback to
ensure that free(message)
On 2011/10/18 17:19:58, antonmuhin wrote:
> hmm, you're saying "Claims ownership of the memory pointed to by 'message' and
> will ensure that free(message) is called." in PortMap::PostMessage.
> 
> Contradiction?

Nope.  PortMap::PostMessage claims ownership from it's caller and hands off
ownership to the callback.

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:220: typedef bool
(*Dart_PostMessageCallback)(Dart_Isolate dest_isolate,
On 2011/10/18 17:19:58, antonmuhin wrote:
> what's the semantics of returned value?

Added:

// The callback should return true if the message was successfully
// posted.

http://codereview.chromium.org/8297004/diff/15001/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/lib/isolate.cc#newc...
runtime/lib/isolate.cc:206: // Keep listening until there are no active receive
ports.
On 2011/10/18 17:19:58, antonmuhin wrote:
> nit: this comment can probably go away now.

Done.

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:104: Isolate* isolate= Isolate::Current();
On 2011/10/18 17:19:58, antonmuhin wrote:
> nit: space before =

Fixed.

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:152: // exception so taht eh embedder can choose how
to handle this
On 2011/10/18 17:19:58, antonmuhin wrote:
> nit: so [that] [the] embedder can choose

Done.

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:167: // Keep listening until there are no active
receive ports.
On 2011/10/18 17:19:58, antonmuhin wrote:
> nit: ditto for comment.

Done.

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/isolate.h#newcode49
runtime/vm/isolate.h:49: Dart_PostMessageCallback post_message_callback() const
{
On 2011/10/18 17:19:58, antonmuhin wrote:
> will you add a TODO to handle concurrent update of isolate's callbacks and
> message sending?

I added a comment in dart_api.h declaiming responsibility...

// Most embedders will only call this function once, before isolate
// execution begins.  If this function is called after isolate
// execution begins, the embedder is responsible for threading issues.

Is that not enough?

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/message_queue.cc...
runtime/vm/message_queue.cc:43: if (result == NULL) {
On 2011/10/18 17:19:58, antonmuhin wrote:
> should it be if or while?  Wait is usually in the loop, but you might have
> stronger guarantees (but I would keep it in the loop anyway).

This should be an 'if'.  The loop is in the caller.

[Anton Muhin, 2011-10-18 17:40:53]

LGTM too.

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/include/dart_api.h#...
runtime/include/dart_api.h:220: typedef bool
(*Dart_PostMessageCallback)(Dart_Isolate dest_isolate,
Great.  Minor correction: not posted, but something like queued or processed.

On 2011/10/18 17:33:40, turnidge wrote:
> On 2011/10/18 17:19:58, antonmuhin wrote:
> > what's the semantics of returned value?
> 
> Added:
> 
> // The callback should return true if the message was successfully
> // posted.

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/message_queue.cc
File runtime/vm/message_queue.cc (right):

http://codereview.chromium.org/8297004/diff/15001/runtime/vm/message_queue.cc...
runtime/vm/message_queue.cc:43: if (result == NULL) {
On 2011/10/18 17:33:40, turnidge wrote:
> On 2011/10/18 17:19:58, antonmuhin wrote:
> > should it be if or while?  Wait is usually in the loop, but you might have
> > stronger guarantees (but I would keep it in the loop anyway).
> 
> This should be an 'if'.  The loop is in the caller.

Oh, I see, thanks.