Revert making HandleWatcher block until no longer waiting on pipe (r285266).

Revert making HandleWatcher block until no longer waiting on pipe (r285266).

That change causes any thread that uses HandleWatcher to block and wait on the watcher thread (which uses mojo message pump). With the new EDK, this causes hangs on shutdown if we share the IO thread between chrome and the EDK, since if the destruction of HandleWatcher happens on the IO thread, the handle watcher's mojo pump will never wake up (since the IO thread is blocked, so it doesn't get to deliver the control byte read event to MojoMessagePump). Since the EDK already handles the case fine by returning error codes, no need to make threads block.

BUG=394886, 561803
Committed: https://crrev.com/daeca3e623dbefaa485889eff9f7712e0f572d51
Cr-Commit-Position: refs/heads/master@{#362190}

[jam, 2015-11-26 01:24:28]

Description was changed from

==========
Revert making HandleWatcher block until no longer waiting on pipe (r285266).

That change causes any thread that uses HandleWatcher to block and wait on the
watcher thread (which uses mojo message pump). With the new EDK, this causes
hangs if we share the IO thread between chrome and the EDK, since if the
destruction of HandleWatcher happens on the IO thread, the handle watcher's mojo
pump will never wake up. Since the EDK already handles the case fine by
returning error codes, no need to make threads block.

BUG=394886, 561803
==========

to

==========
Revert making HandleWatcher block until no longer waiting on pipe (r285266).

That change causes any thread that uses HandleWatcher to block and wait on the
watcher thread (which uses mojo message pump). With the new EDK, this causes
hangs on shutdown if we share the IO thread between chrome and the EDK, since if
the destruction of HandleWatcher happens on the IO thread, the handle watcher's
mojo pump will never wake up (since the IO thread is blocked, so it doesn't get
to deliver the control byte read event to MojoMessagePump). Since the EDK
already handles the case fine by returning error codes, no need to make threads
block.

BUG=394886, 561803
==========

[jam, 2015-11-26 01:24:28]

jam@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-11-26 01:48:51]

What happens if there is a waitmany in progress and one closes or
transfers the pipe? Are there tests that ensure this works ok? Also,
since the new edk isn't the default do we need to worry about what the
old edk does until the new edk is the defalut?

  -Scott

On Wed, Nov 25, 2015 at 5:24 PM,  <jam@chromium.org> wrote:
> Reviewers: sky
> CL: https://codereview.chromium.org/1475983004/
>
> Description:
> Revert making HandleWatcher block until no longer waiting on pipe (r285266).
>
> That change causes any thread that uses HandleWatcher to block and wait on
> the
> watcher thread (which uses mojo message pump). With the new EDK, this causes
> hangs on shutdown if we share the IO thread between chrome and the EDK,
> since if
> the destruction of HandleWatcher happens on the IO thread, the handle
> watcher's
> mojo pump will never wake up (since the IO thread is blocked, so it doesn't
> get
> to deliver the control byte read event to MojoMessagePump). Since the EDK
> already handles the case fine by returning error codes, no need to make
> threads
> block.
>
> BUG=394886, 561803
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+66, -140 lines):
>   M base/threading/thread_restrictions.h
>   M
> mojo/android/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java
>   M mojo/message_pump/handle_watcher.h
>   M mojo/message_pump/handle_watcher.cc
>   M mojo/message_pump/handle_watcher_unittest.cc
>   M mojo/message_pump/message_pump_mojo.cc
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2015-11-26 02:13:40]

On 2015/11/26 01:48:51, sky wrote:
> What happens if there is a waitmany in progress and one closes or
> transfers the pipe?

MOJO_RESULT_CANCELLED is sent as a result when a pipe is closed/transferred
(MessagePipeDispatcher::CancelAllAwakablesNoLock in both old and new EDK)

> Are there tests that ensure this works ok?

I don't think so, since your cl changing this behavior didn't remove such tests.

> Also,
> since the new edk isn't the default do we need to worry about what the
> old edk does until the new edk is the defalut?

both old and new EDKs don't recycle message IDs until they wrap around. Is that
what you were referring to?

The old edk doesn't have this problem because message pipes in the same process
don't use the IO thread. but this change shouldn't be harmful there.
> 
>   -Scott

[Anand Mistry (off Chromium), 2015-11-26 02:49:03]

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

[Anand Mistry (off Chromium), 2015-11-26 02:49:05]

Sorry for the drive-by.

https://codereview.chromium.org/1475983004/diff/1/mojo/message_pump/handle_wa...
File mojo/message_pump/handle_watcher_unittest.cc (left):

https://codereview.chromium.org/1475983004/diff/1/mojo/message_pump/handle_wa...
mojo/message_pump/handle_watcher_unittest.cc:443:
TEST(HandleWatcherCleanEnvironmentTest, StressTest) {
Can you keep this test? If HandleWatcher is functioning correctly, this should
continue to pass, right?

[jam, 2015-11-26 03:32:02]

https://codereview.chromium.org/1475983004/diff/1/mojo/message_pump/handle_wa...
File mojo/message_pump/handle_watcher_unittest.cc (left):

https://codereview.chromium.org/1475983004/diff/1/mojo/message_pump/handle_wa...
mojo/message_pump/handle_watcher_unittest.cc:443:
TEST(HandleWatcherCleanEnvironmentTest, StressTest) {
On 2015/11/26 02:49:04, Anand Mistry wrote:
> Can you keep this test? If HandleWatcher is functioning correctly, this should
> continue to pass, right?

ah good point thanks. I had blindly reverted it. Brought back and verified it
still passes.

[sky, 2015-11-30 16:10:09]

Update the doc of mojo/public/c/environment/async_waiter.h and LGTM

[jam, 2015-11-30 16:38:14]

The CQ bit was checked by jam@chromium.org

[jam, 2015-11-30 16:38:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1475983004/#ps40001
(title: "update comment (undo change in r283888")

[commit-bot: I haz the power, 2015-11-30 16:38:43]

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

[jam, 2015-11-30 16:43:13]

On 2015/11/30 16:10:09, sky wrote:
> Update the doc of mojo/public/c/environment/async_waiter.h and LGTM

Done, thanks for bringing that up

[jam, 2015-11-30 16:43:14]

On 2015/11/30 16:10:09, sky wrote:
> Update the doc of mojo/public/c/environment/async_waiter.h and LGTM

Done, thanks for bringing that up

[commit-bot: I haz the power, 2015-11-30 19:25:14]

Description was changed from

==========
Revert making HandleWatcher block until no longer waiting on pipe (r285266).

That change causes any thread that uses HandleWatcher to block and wait on the
watcher thread (which uses mojo message pump). With the new EDK, this causes
hangs on shutdown if we share the IO thread between chrome and the EDK, since if
the destruction of HandleWatcher happens on the IO thread, the handle watcher's
mojo pump will never wake up (since the IO thread is blocked, so it doesn't get
to deliver the control byte read event to MojoMessagePump). Since the EDK
already handles the case fine by returning error codes, no need to make threads
block.

BUG=394886, 561803
==========

to

==========
Revert making HandleWatcher block until no longer waiting on pipe (r285266).

That change causes any thread that uses HandleWatcher to block and wait on the
watcher thread (which uses mojo message pump). With the new EDK, this causes
hangs on shutdown if we share the IO thread between chrome and the EDK, since if
the destruction of HandleWatcher happens on the IO thread, the handle watcher's
mojo pump will never wake up (since the IO thread is blocked, so it doesn't get
to deliver the control byte read event to MojoMessagePump). Since the EDK
already handles the case fine by returning error codes, no need to make threads
block.

BUG=394886, 561803
==========

[commit-bot: I haz the power, 2015-11-30 19:25:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-30 19:26:22]

Description was changed from

==========
Revert making HandleWatcher block until no longer waiting on pipe (r285266).

That change causes any thread that uses HandleWatcher to block and wait on the
watcher thread (which uses mojo message pump). With the new EDK, this causes
hangs on shutdown if we share the IO thread between chrome and the EDK, since if
the destruction of HandleWatcher happens on the IO thread, the handle watcher's
mojo pump will never wake up (since the IO thread is blocked, so it doesn't get
to deliver the control byte read event to MojoMessagePump). Since the EDK
already handles the case fine by returning error codes, no need to make threads
block.

BUG=394886, 561803
==========

to

==========
Revert making HandleWatcher block until no longer waiting on pipe (r285266).

That change causes any thread that uses HandleWatcher to block and wait on the
watcher thread (which uses mojo message pump). With the new EDK, this causes
hangs on shutdown if we share the IO thread between chrome and the EDK, since if
the destruction of HandleWatcher happens on the IO thread, the handle watcher's
mojo pump will never wake up (since the IO thread is blocked, so it doesn't get
to deliver the control byte read event to MojoMessagePump). Since the EDK
already handles the case fine by returning error codes, no need to make threads
block.

BUG=394886, 561803

Committed: https://crrev.com/daeca3e623dbefaa485889eff9f7712e0f572d51
Cr-Commit-Position: refs/heads/master@{#362190}
==========

[commit-bot: I haz the power, 2015-11-30 19:26:23]

Patchset 3 (id:??) landed as
https://crrev.com/daeca3e623dbefaa485889eff9f7712e0f572d51
Cr-Commit-Position: refs/heads/master@{#362190}