[mojo-edk] Add MojoWatch and MojoCancelWatch APIs

[mojo-edk] Add MojoWatch and MojoCancelWatch APIs

This adds MojoWatch() and MojoCancelWatch() APIs to support
efficient asynchronous handle event notifications.

BUG=590495
Committed: https://crrev.com/49b69e6b27a12426ba56b6b5271ccec9b54c94c2
Cr-Commit-Position: refs/heads/master@{#378677}

[Ken Rockot(use gerrit already), 2016-02-28 02:22:45]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2016-02-28 02:23:30]

rockot@chromium.org changed reviewers:
+ amistry@chromium.org

[Ken Rockot(use gerrit already), 2016-02-28 02:23:31]

ptal

[Ken Rockot(use gerrit already), 2016-02-28 02:26:46]

Patchset #1 (id:20001) has been deleted

[Ken Rockot(use gerrit already), 2016-02-28 06:20:11]

Patchset #3 (id:80001) has been deleted

[Ken Rockot(use gerrit already), 2016-02-28 06:25:03]

Alright so a proper solution for Mojo system re-entrancy is now in place.

It's pretty straightforward: Core is the master EDK entry point, so its relevant
methods can construct a context object on the stack before calling to a
dispatcher. The context can have finalizers attached to it, and it will only run
finalizers immediately before exiting an EDK API call after any locks have been
released. If a dispatcher ever wants to run a client callback, it adds a
finalizer to the RequestContext on its stack.

Unfortunately this did mean plumbing a RequestContext* through a bunch of places
which adds a ton of noise to the CL.

[Anand Mistry (off Chromium), 2016-02-29 12:03:35]

The RequestContext plumbing adds a huge amount of noise to this change. What do
you think about storing it in TLS instead? And if you go that route, you could
implement a custom Awakable and use the existing awakables mechanism instead and
get DataPipe support "for free".

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/dispat...
File mojo/edk/system/dispatcher.h (right):

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/dispat...
mojo/edk/system/dispatcher.h:50: static const size_t kFinalizersStackBufferSize
= 4;
Shouldn't these be private?

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/dispat...
mojo/edk/system/dispatcher.h:59: void AddFinalizer(const base::Closure&
callback);
Hmm... Since you're already making an effort to avoid allocs, and these closure
are always made with the same signature, maybe this should just take the watch
function and its arguments (and rename accordingly). Maybe it's premature, but
the cost of malloc/free and the CAS (increment) operation in refcounting (inside
base::Callback) are significant and paid on every inbound message.

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/watche...
File mojo/edk/system/watcher_set.cc (right):

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.cc:8: #include "base/containers/stack_container.h"
Unused?

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.cc:13: struct HandleSignalsState;
Remove and #include "mojo/edk/system/handle_signals_state.h".

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.cc:28: MojoHandleSignalsState signals_state;
Given that HandleSignalsSate inherits from MojoHandleSignalsState, you should be
able to just pass |state| through. You might need a static_cast in
MaybeNotifyWatcher(), but you get the advantage of avoiding the manual copy
(twice) and get to use HandleSignalsSate::satisfies().

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
File mojo/public/c/system/functions.h (right):

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
mojo/public/c/system/functions.h:148: //     TODO: Perhaps we could also support
data pipes here.
If you want to replace wait, you need to support data pipe. Looking at
MessagePipe, it looks relatively simple to add.

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
mojo/public/c/system/functions.h:154: //     the watch later using
|MojoCancelWatch().|
nit: |MojoCancelWatch()|.

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
mojo/public/c/system/functions.h:171: //   The callback may be called at any
time with one of the following result
at any time... on any thread...

I think. I didn't see anything that ties callbacks to threads.

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
mojo/public/c/system/functions.h:194: //     |MOJO_RESULT_OK| if the watch
corresponding to |context| was cancelled.
Can you clarify what happens if the watch is in the process of being run. It's
an important note when dealing with task cancellation.

[Ken Rockot(use gerrit already), 2016-02-29 20:59:06]

Thanks. I think I've addressed everything. To reduce redundant code in
dispatchers for now I've added a WatcherSet to AwakableList, and AwakableList
always notifies its watchers at the appropriate time. Eventually we should be
able to delete AwakableList but I avoided doing that here.

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/dispat...
File mojo/edk/system/dispatcher.h (right):

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/dispat...
mojo/edk/system/dispatcher.h:50: static const size_t kFinalizersStackBufferSize
= 4;
On 2016/02/29 at 12:03:35, Anand Mistry wrote:
> Shouldn't these be private?

Done

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/dispat...
mojo/edk/system/dispatcher.h:59: void AddFinalizer(const base::Closure&
callback);
On 2016/02/29 at 12:03:35, Anand Mistry wrote:
> Hmm... Since you're already making an effort to avoid allocs, and these
closure are always made with the same signature, maybe this should just take the
watch function and its arguments (and rename accordingly). Maybe it's premature,
but the cost of malloc/free and the CAS (increment) operation in refcounting
(inside base::Callback) are significant and paid on every inbound message.

I agree, base::Callback is overkill. To support reliable cancellation I've made
Watcher a ref-counted thing, which I think is OK since it's not exposed in many
places. Anyway, we now pass a ref to the Watcher and the arguments to invoke its
callback with.

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/watche...
File mojo/edk/system/watcher_set.cc (right):

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.cc:8: #include "base/containers/stack_container.h"
On 2016/02/29 at 12:03:35, Anand Mistry wrote:
> Unused?

Removed

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.cc:13: struct HandleSignalsState;
On 2016/02/29 at 12:03:35, Anand Mistry wrote:
> Remove and #include "mojo/edk/system/handle_signals_state.h".

Done

https://codereview.chromium.org/1748503002/diff/120001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.cc:28: MojoHandleSignalsState signals_state;
On 2016/02/29 at 12:03:35, Anand Mistry wrote:
> Given that HandleSignalsSate inherits from MojoHandleSignalsState, you should
be able to just pass |state| through. You might need a static_cast in
MaybeNotifyWatcher(), but you get the advantage of avoiding the manual copy
(twice) and get to use HandleSignalsSate::satisfies().

Ah right... static_cast :) Done

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
File mojo/public/c/system/functions.h (right):

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
mojo/public/c/system/functions.h:148: //     TODO: Perhaps we could also support
data pipes here.
On 2016/02/29 at 12:03:35, Anand Mistry wrote:
> If you want to replace wait, you need to support data pipe. Looking at
MessagePipe, it looks relatively simple to add.

Yeah, was just trying to avoid more noise in the CL since we're not actually
deleting wait yet. However given your TLS suggestion it's even more trivial to
add this support. So, Done

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
mojo/public/c/system/functions.h:154: //     the watch later using
|MojoCancelWatch().|
On 2016/02/29 at 12:03:35, Anand Mistry wrote:
> nit: |MojoCancelWatch()|.

Done

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
mojo/public/c/system/functions.h:171: //   The callback may be called at any
time with one of the following result
On 2016/02/29 at 12:03:35, Anand Mistry wrote:
> at any time... on any thread...
> 
> I think. I didn't see anything that ties callbacks to threads.

Done

https://codereview.chromium.org/1748503002/diff/120001/mojo/public/c/system/f...
mojo/public/c/system/functions.h:194: //     |MOJO_RESULT_OK| if the watch
corresponding to |context| was cancelled.
On 2016/02/29 at 12:03:35, Anand Mistry wrote:
> Can you clarify what happens if the watch is in the process of being run. It's
an important note when dealing with task cancellation.

Done

[Anand Mistry (off Chromium), 2016-03-01 07:30:40]

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_producer_dispatcher.cc (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_producer_dispatcher.cc:462: void
DataPipeProducerDispatcher::OnPortStatusChanged() {
Don't you need a RequestContext here as well? Same for
DataPipeConsumerDispatcher.

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
mojo/edk/system/request_context.cc:14: base::ThreadLocalPointer<RequestContext>
g_current_context;
Needs to be a LazyInstance, otherwise you're introducing a static initializer.

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
File mojo/edk/system/request_context.h (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
mojo/edk/system/request_context.h:19: // NOTE: It is legal to construct a
RequestContext while another one already
Are there any specific cases where this happens right now? I'd like to know if
you can remove this and always require that there's only one RequestContext
active on a thread.

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
mojo/edk/system/request_context.h:67: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
File mojo/edk/system/watcher.cc (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
mojo/edk/system/watcher.cc:25: void
Watcher::NotifyForStateChange(MojoHandleSignalsState signals_state) {
Can't you use mojo::HandleSignalsState here as well, then use .can_satisfy() in
the condition below? Really, any place you can use it, you should. It's only
data that comes from the user via the public API that needs to be
MojoHandleSignalsState.

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watcher.h
File mojo/edk/system/watcher.h (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
mojo/edk/system/watcher.h:68: // Guards |is_valid_|.
s/is_valid_/is_cancelled_/

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
File mojo/edk/system/watcher_set.cc (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.cc:52: it->second->Cancel();
Hmm. There's a potential deadlock here. If on thread |A| you try and remove a
watcher, and on thread |B|, the callback is running and tries to add a watcher,
you have a deadlock.
Specifically:
A - has acquired dispatcher lock, tries to acquire Watcher::lock_
B - has Watcher::lock_, tries to acquire dispatcher lock

At the very least, TSAN will complain about lock inversion if you try it.

[Ken Rockot(use gerrit already), 2016-03-01 08:17:44]

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/data_p...
File mojo/edk/system/data_pipe_producer_dispatcher.cc (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/data_p...
mojo/edk/system/data_pipe_producer_dispatcher.cc:462: void
DataPipeProducerDispatcher::OnPortStatusChanged() {
On 2016/03/01 at 07:30:40, Anand Mistry wrote:
> Don't you need a RequestContext here as well? Same for
DataPipeConsumerDispatcher.

Oops, done and done

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
mojo/edk/system/request_context.cc:14: base::ThreadLocalPointer<RequestContext>
g_current_context;
On 2016/03/01 at 07:30:40, Anand Mistry wrote:
> Needs to be a LazyInstance, otherwise you're introducing a static initializer.

done

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
File mojo/edk/system/request_context.h (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
mojo/edk/system/request_context.h:19: // NOTE: It is legal to construct a
RequestContext while another one already
On 2016/03/01 at 07:30:40, Anand Mistry wrote:
> Are there any specific cases where this happens right now? I'd like to know if
you can remove this and always require that there's only one RequestContext
active on a thread.

So if you WriteMessage to a pipe endpoint whose peer is in the same process,
OnPortStatusChanged will be called synchronously on peer's dispatcher. But
OnPortStatusChanged may also be called as a result of EDK entry via IPC arrival
on a NodeChannel, so it needs to ensure it has a RequestContext in that case
too.

I couldn't decide between doing this, or just adding a RequestContext somewhere
in NodeController (basically anywhere Node::AcceptMessage can be called) and
removing them from OnPortStatusChanged. I went with this because it seemed
harder to get wrong. Not sure that is the right call though. WDYT?

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
mojo/edk/system/request_context.h:67: };
On 2016/03/01 at 07:30:40, Anand Mistry wrote:
> DISALLOW_COPY_AND_ASSIGN

Done

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
File mojo/edk/system/watcher.cc (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
mojo/edk/system/watcher.cc:25: void
Watcher::NotifyForStateChange(MojoHandleSignalsState signals_state) {
On 2016/03/01 at 07:30:40, Anand Mistry wrote:
> Can't you use mojo::HandleSignalsState here as well, then use .can_satisfy()
in the condition below? Really, any place you can use it, you should. It's only
data that comes from the user via the public API that needs to be
MojoHandleSignalsState.

Done here and everywhere else it's possible

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watcher.h
File mojo/edk/system/watcher.h (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
mojo/edk/system/watcher.h:68: // Guards |is_valid_|.
On 2016/03/01 at 07:30:40, Anand Mistry wrote:
> s/is_valid_/is_cancelled_/

done

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
File mojo/edk/system/watcher_set.cc (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.cc:52: it->second->Cancel();
On 2016/03/01 at 07:30:40, Anand Mistry wrote:
> Hmm. There's a potential deadlock here. If on thread |A| you try and remove a
watcher, and on thread |B|, the callback is running and tries to add a watcher,
you have a deadlock.
> Specifically:
> A - has acquired dispatcher lock, tries to acquire Watcher::lock_
> B - has Watcher::lock_, tries to acquire dispatcher lock
> 
> At the very least, TSAN will complain about lock inversion if you try it.

Good catch. I've changed this so that cancellation also uses a finalizer.

[Ken Rockot(use gerrit already), 2016-03-01 15:54:44]

FYI I went ahead and made the change to RequestContext usage to enforce that
only one may exist. 

I like it better, but you can decide which one you like. I think documenting why
it exists where it does in NodeController is sufficient mitigation for future
confusion.

Patch set 7 is just patch set 6 plus RequestContext exclusivity.

[Ken Rockot(use gerrit already), 2016-03-01 15:57:08]

Patchset #7 (id:180001) has been deleted

[Ken Rockot(use gerrit already), 2016-03-01 17:18:50]

Patchset #7 (id:200001) has been deleted

[Anand Mistry (off Chromium), 2016-03-02 01:50:36]

LGTM with nits and...

I prefer what you had previously where you allowed (unused) nested contexts. I
think having the restriction on nesting is harder to reason about correctness,
and potentially buggy. I'd rather just allow it and document what happens when
you do so. I'll leave the decision up to you.

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
File mojo/edk/system/request_context.h (right):

https://codereview.chromium.org/1748503002/diff/140001/mojo/edk/system/reques...
mojo/edk/system/request_context.h:19: // NOTE: It is legal to construct a
RequestContext while another one already
On 2016/03/01 08:17:44, Ken Rockot wrote:
> On 2016/03/01 at 07:30:40, Anand Mistry wrote:
> > Are there any specific cases where this happens right now? I'd like to know
if
> you can remove this and always require that there's only one RequestContext
> active on a thread.
> 
> So if you WriteMessage to a pipe endpoint whose peer is in the same process,
> OnPortStatusChanged will be called synchronously on peer's dispatcher. But
> OnPortStatusChanged may also be called as a result of EDK entry via IPC
arrival
> on a NodeChannel, so it needs to ensure it has a RequestContext in that case
> too.
> 
> I couldn't decide between doing this, or just adding a RequestContext
somewhere
> in NodeController (basically anywhere Node::AcceptMessage can be called) and
> removing them from OnPortStatusChanged. I went with this because it seemed
> harder to get wrong. Not sure that is the right call though. WDYT?

This approach is good. The thing that confused me a bit was that you end up in a
situation where there'a RequestContext on the stack that is completely unused.
But I think this is the way to go. In fact, I think it's desirable to unwind the
stack as much as possible before executing any callbacks.

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/reques...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/reques...
mojo/edk/system/request_context.cc:7: #include "base/lazy_instance.h"
#include "base/logging.h"

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/watche...
File mojo/edk/system/watcher_set.h (right):

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.h:10: #include "base/callback.h"
#include "base/macros.h"

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.h:12: #include "base/synchronization/lock.h"
Unused

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.h:41: MojoResult Remove(uintptr_t context);
For consistency, add a comment.

[Ken Rockot(use gerrit already), 2016-03-02 02:09:39]

Cool. I thought about it some more and I agree with you.

Initially I was thinking it would be easier to detect bugs if we didn't allow
nesting, but allowing nesting really only leaves open one interesting class of
bugs, which is when a RequestContext is needed but doesn't exist. That's a nice
property, and such bugs seem like they would be easy to find and fix if they
ever arise, so I've reverted back to allowing nesting.

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/reques...
File mojo/edk/system/request_context.cc (right):

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/reques...
mojo/edk/system/request_context.cc:7: #include "base/lazy_instance.h"
On 2016/03/02 at 01:50:36, Anand Mistry wrote:
> #include "base/logging.h"

done

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/watche...
File mojo/edk/system/watcher_set.h (right):

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.h:10: #include "base/callback.h"
On 2016/03/02 at 01:50:36, Anand Mistry wrote:
> #include "base/macros.h"

done

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.h:12: #include "base/synchronization/lock.h"
On 2016/03/02 at 01:50:36, Anand Mistry wrote:
> Unused

removed

https://codereview.chromium.org/1748503002/diff/220001/mojo/edk/system/watche...
mojo/edk/system/watcher_set.h:41: MojoResult Remove(uintptr_t context);
On 2016/03/02 at 01:50:36, Anand Mistry wrote:
> For consistency, add a comment.

done

[Ken Rockot(use gerrit already), 2016-03-02 02:09:50]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-03-02 02:09:50]

The patchset sent to the CQ was uploaded after l-g-t-m from amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/1748503002/#ps240001
(title: "revert RequestContext usage, nits")

[commit-bot: I haz the power, 2016-03-02 02:10:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748503002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748503002/240001

[commit-bot: I haz the power, 2016-03-02 03:46:46]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2016-03-02 03:49:03]

Description was changed from

==========
[mojo-edk] Add MojoWatch and MojoCancelWatch APIs

This adds MojoWatch() and MojoCancelWatch() APIs to support
efficient asynchronous handle event notifications.

BUG=590495
==========

to

==========
[mojo-edk] Add MojoWatch and MojoCancelWatch APIs

This adds MojoWatch() and MojoCancelWatch() APIs to support
efficient asynchronous handle event notifications.

BUG=590495

Committed: https://crrev.com/49b69e6b27a12426ba56b6b5271ccec9b54c94c2
Cr-Commit-Position: refs/heads/master@{#378677}
==========

[commit-bot: I haz the power, 2016-03-02 03:49:04]

Patchset 8 (id:??) landed as
https://crrev.com/49b69e6b27a12426ba56b6b5271ccec9b54c94c2
Cr-Commit-Position: refs/heads/master@{#378677}