Use WaitableEvents to wake up sync IPC waiting

Use WaitableEvents to wake up sync IPC waiting

Changes mojo::WaitSet and mojo::SyncHandleRegistry to support waiting on
arbitrary user events concurrently with Mojo handle state changes.

Reverts IPC::SyncChannel to use WaitableEvent for forced wakeups in lieu
of signaling with one-off message pipes (i.e. "MojoEvents", which have
been removed here as well.)

Also uses WaitableEvent for blocking in the bindings' MultiplexRouter and
IPC Channel's internal equivalent.

BUG=698460
Review-Url: https://codereview.chromium.org/2754143005
Cr-Commit-Position: refs/heads/master@{#459486}
Committed: https://chromium.googlesource.com/chromium/src/+/b62e2e3d17205ea1e128d85fb6211192e62855d9

[Ken Rockot(use gerrit already), 2017-03-17 20:05:06]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 20:05:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 21:44:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-17 21:44:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2017-03-20 19:34:37]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 19:35:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 20:52:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 20:52:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ken Rockot(use gerrit already), 2017-03-21 07:44:27]

Patchset #3 (id:40001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-22 19:53:43]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 19:54:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-03-22 19:55:04]

Patchset #3 (id:60001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-22 19:55:09]

Patchset #2 (id:20001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-22 19:55:13]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2017-03-22 19:56:46]

Description was changed from

==========
Use WaitableEvents to wake up sync IPC waiting

Changes mojo::WaitSet and mojo::SyncHandleRegistry to support waiting on
arbitrary user events concurrently with Mojo handle state changes.

Reverts IPC::SyncChannel to use WaitableEvent for forced wakeups in lieu
of signaling with one-off message pipes (i.e. "MojoEvents", which have
been removed here as well.)

BUG=698460
==========

to

==========
Use WaitableEvents to wake up sync IPC waiting

Changes mojo::WaitSet and mojo::SyncHandleRegistry to support waiting on
arbitrary user events concurrently with Mojo handle state changes.

Reverts IPC::SyncChannel to use WaitableEvent for forced wakeups in lieu
of signaling with one-off message pipes (i.e. "MojoEvents", which have
been removed here as well.)

Also uses WaitableEvent for blocking in the bindings' MultiplexRouter and
IPC Channel's internal equivalent.

BUG=698460
==========

[Ken Rockot(use gerrit already), 2017-03-22 19:58:31]

rockot@chromium.org changed reviewers:
+ jam@chromium.org, yzshen@chromium.org

[Ken Rockot(use gerrit already), 2017-03-22 19:58:32]

PTAL

Yuzhu for //mojo IPC mojo bootstrap
John for the rest of IPC

[commit-bot: I haz the power, 2017-03-22 21:08:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 21:08:42]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jam, 2017-03-23 00:44:40]

should I wait for the the IPC test failures in case they change this?

my one nit is to add more comment in IPC::SyncCHannel, i.e. why is there outer
event?

[Ken Rockot(use gerrit already), 2017-03-23 01:32:40]

On 2017/03/23 at 00:44:40, jam wrote:
> should I wait for the the IPC test failures in case they change this?
> 

Feel free to review or wait - the breakage must be something incredibly subtle.
I cannot reproduce it on any platform under any conditions, but clearly there's
a problem.

> my one nit is to add more comment in IPC::SyncCHannel, i.e. why is there outer
event?

Will do

[Ken Rockot(use gerrit already), 2017-03-23 03:43:46]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 03:44:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-03-23 03:52:27]

Alright! Figured out the bug.

One of the test classes (ChannelProxyRunner) was destroying its WaitableEvent
before the SyncChannel which was watching it with a WaitableEventWatcher.

This was apparently not an issue before because [unknown reasons - probably
luck].

Here it was manifesting as test child processes exiting unexpectedly with an
unhandled STATUS_THREADPOOL_HANDLE_EXCEPTION. Only on Windows 7. Special thanks
to the amazing swarming client and LOG(ERROR) for making this possible to debug
in under several hours.

[commit-bot: I haz the power, 2017-03-23 04:01:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 04:01:23]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Ken Rockot(use gerrit already), 2017-03-23 04:02:16]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 04:02:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 04:18:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 04:18:56]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Ken Rockot(use gerrit already), 2017-03-23 13:52:28]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 13:52:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 14:14:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 14:14:30]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-03-23 20:15:51]

LGTM with a few nits.

https://codereview.chromium.org/2754143005/diff/120001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/ipc/ipc_sync_channel.c...
ipc/ipc_sync_channel.cc:660: base::WaitableEventWatcher* outer_watcher =
nullptr;
Does it make sense to group these three things into a struct, so it is easier to
init or pass around?

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:112:
sync_message_event_->Signal();
Is it cheap enough to always create the event for every interface? (This method
is called whenever an endpoint's peer is closed)

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/sync_event_watcher.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/sync_event_watcher.cc:42: bool result =
registry_->WatchAllHandles(should_stop_array, 2);
nit: Maybe WatchAllHandles() could be renamed to WatchAll().

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/sync_handle_registry.h (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/sync_handle_registry.h:70:
std::map<base::WaitableEvent*, base::Closure> events_;
It is a little weird to use unordered_map for one thing and map for the other.
Can we unify them? I am fine with either one (without any data support).

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/system...
File mojo/public/cpp/system/wait_set.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/system...
mojo/public/cpp/system/wait_set.cc:165: // returning the results immediately so
as to not avoid potentially
nit: "not avoid" -> "avoid"?

[Ken Rockot(use gerrit already), 2017-03-23 22:03:38]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-03-23 22:04:20]

https://codereview.chromium.org/2754143005/diff/120001/ipc/ipc_sync_channel.cc
File ipc/ipc_sync_channel.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/ipc/ipc_sync_channel.c...
ipc/ipc_sync_channel.cc:660: base::WaitableEventWatcher* outer_watcher =
nullptr;
On 2017/03/23 at 20:15:50, yzshen1 wrote:
> Does it make sense to group these three things into a struct, so it is easier
to init or pass around?

Sure, that sounds nice. In fact I've gone and moved all of this logic into a
helper class and I think it's much easier to understand now.

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:112:
sync_message_event_->Signal();
On 2017/03/23 at 20:15:50, yzshen1 wrote:
> Is it cheap enough to always create the event for every interface? (This
method is called whenever an endpoint's peer is closed)

Sure, probably. I guess the use of Optional can be seen as a premature
optimization now, so I'll just always create the event.

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/sync_event_watcher.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/sync_event_watcher.cc:42: bool result =
registry_->WatchAllHandles(should_stop_array, 2);
On 2017/03/23 at 20:15:50, yzshen1 wrote:
> nit: Maybe WatchAllHandles() could be renamed to WatchAll().

Done (Now it's Wait())

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/sync_handle_registry.h (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/sync_handle_registry.h:70:
std::map<base::WaitableEvent*, base::Closure> events_;
On 2017/03/23 at 20:15:50, yzshen1 wrote:
> It is a little weird to use unordered_map for one thing and map for the other.
Can we unify them? I am fine with either one (without any data support).

Done. Based on recent discussions making me feel sad about unordered_map
insertion behavior, and the fact that these are small maps with fast lookup in
any case, they are now both map.

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/system...
File mojo/public/cpp/system/wait_set.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/system...
mojo/public/cpp/system/wait_set.cc:165: // returning the results immediately so
as to not avoid potentially
On 2017/03/23 at 20:15:50, yzshen1 wrote:
> nit: "not avoid" -> "avoid"?

Done

[commit-bot: I haz the power, 2017-03-23 22:05:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-03-23 22:05:29]

Patchset #4 (id:140001) has been deleted

[Ken Rockot(use gerrit already), 2017-03-23 22:05:46]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 22:06:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2017-03-23 22:20:06]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-03-23 22:20:16]

Patchset #4 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-03-23 22:20:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 22:23:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 22:23:06]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[yzshen1, 2017-03-23 22:42:10]

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:112:
sync_message_event_->Signal();
On 2017/03/23 22:04:20, Ken Rockot wrote:
> On 2017/03/23 at 20:15:50, yzshen1 wrote:
> > Is it cheap enough to always create the event for every interface? (This
> method is called whenever an endpoint's peer is closed)
> 
> Sure, probably. I guess the use of Optional can be seen as a premature
> optimization now, so I'll just always create the event.

Sorry I didn't make myself clear. Actually I was thinking maybe it is useful to
keep |event_signalled_| and avoid creating the event unless it is absolutely
necessary. (My last comment sounds like I am suggesting the opposite.)
But if we are confident that event creation is cheap, then I don't have problem
with current approach.

[Ken Rockot(use gerrit already), 2017-03-23 22:57:34]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 22:58:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 00:14:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 00:14:14]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2017-03-24 04:59:55]

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (right):

https://codereview.chromium.org/2754143005/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:112:
sync_message_event_->Signal();
On 2017/03/23 at 22:42:10, yzshen1 wrote:
> On 2017/03/23 22:04:20, Ken Rockot wrote:
> > On 2017/03/23 at 20:15:50, yzshen1 wrote:
> > > Is it cheap enough to always create the event for every interface? (This
> > method is called whenever an endpoint's peer is closed)
> > 
> > Sure, probably. I guess the use of Optional can be seen as a premature
> > optimization now, so I'll just always create the event.
> 
> Sorry I didn't make myself clear. Actually I was thinking maybe it is useful
to keep |event_signalled_| and avoid creating the event unless it is absolutely
necessary. (My last comment sounds like I am suggesting the opposite.)
> But if we are confident that event creation is cheap, then I don't have
problem with current approach.

Oops, I misunderstood. I was thinking we would create the event for every
interface anyway on peer closure. But that's not what you meant. I think it's
worth avoiding event creation for interfaces that never do any sync waiting, so
I've added the flag back.

[yzshen1, 2017-03-24 16:41:47]

https://codereview.chromium.org/2754143005/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (right):

https://codereview.chromium.org/2754143005/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:196:
base::WaitableEvent::InitialState::NOT_SIGNALED);
If the flag |event_signaled_| is already set to true here, we need to signal the
event.

[Ken Rockot(use gerrit already), 2017-03-24 16:43:52]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[Ken Rockot(use gerrit already), 2017-03-24 16:43:55]

https://codereview.chromium.org/2754143005/diff/200001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (right):

https://codereview.chromium.org/2754143005/diff/200001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/lib/multiplex_router.cc:196:
base::WaitableEvent::InitialState::NOT_SIGNALED);
On 2017/03/24 at 16:41:47, yzshen1 wrote:
> If the flag |event_signaled_| is already set to true here, we need to signal
the event.

Done

[commit-bot: I haz the power, 2017-03-24 16:44:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2017-03-24 16:45:06]

lgtm

[commit-bot: I haz the power, 2017-03-24 18:09:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 18:09:42]

Dry run: This issue passed the CQ dry run.

[jam, 2017-03-24 18:16:07]

lgtm

[Ken Rockot(use gerrit already), 2017-03-24 18:16:23]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2017-03-24 18:17:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 18:36:48]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490379383393000,
"parent_rev": "704ca0cfcc046d199d31e6502639bc94b89fddb0", "commit_rev":
"b62e2e3d17205ea1e128d85fb6211192e62855d9"}

[commit-bot: I haz the power, 2017-03-24 18:37:41]

Description was changed from

==========
Use WaitableEvents to wake up sync IPC waiting

Changes mojo::WaitSet and mojo::SyncHandleRegistry to support waiting on
arbitrary user events concurrently with Mojo handle state changes.

Reverts IPC::SyncChannel to use WaitableEvent for forced wakeups in lieu
of signaling with one-off message pipes (i.e. "MojoEvents", which have
been removed here as well.)

Also uses WaitableEvent for blocking in the bindings' MultiplexRouter and
IPC Channel's internal equivalent.

BUG=698460
==========

to

==========
Use WaitableEvents to wake up sync IPC waiting

Changes mojo::WaitSet and mojo::SyncHandleRegistry to support waiting on
arbitrary user events concurrently with Mojo handle state changes.

Reverts IPC::SyncChannel to use WaitableEvent for forced wakeups in lieu
of signaling with one-off message pipes (i.e. "MojoEvents", which have
been removed here as well.)

Also uses WaitableEvent for blocking in the bindings' MultiplexRouter and
IPC Channel's internal equivalent.

BUG=698460

Review-Url: https://codereview.chromium.org/2754143005
Cr-Commit-Position: refs/heads/master@{#459486}
Committed:
https://chromium.googlesource.com/chromium/src/+/b62e2e3d17205ea1e128d85fb621...
==========

[commit-bot: I haz the power, 2017-03-24 18:37:43]

Committed patchset #6 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/b62e2e3d17205ea1e128d85fb621...