Take extra lock in base::WaitableEvent::WaitMany.

Take extra lock in base::WaitableEvent::WaitMany.

Currently WaitMany doesn't ensure that the corresponding |Signal| is done
when the WaitMany call is returned.
That is problematic when we share the event over threads and want to wait
for detaching it in order to delete it after that.

BUG=397435
R=agl@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288005

[agl, 2014-07-25 17:32:46]

Is the situation that you have a thread waiting on a WaitableEvent and you want
that thread to destroy the WaitableEvent once it wakes up?

This change adds a *lot* more synchronisation points in Chrome and we seemingly
haven't needed it in all the years so far. I suspect you might not be solving
this problem in the best way.

https://codereview.chromium.org/419773002/diff/1/base/synchronization/waitabl...
File base/synchronization/waitable_event_posix.cc (right):

https://codereview.chromium.org/419773002/diff/1/base/synchronization/waitabl...
base/synchronization/waitable_event_posix.cc:293: // We acquire and release the
lock in order to ensure that the
It doesn't seem reasonable that WaitMany has this property, but Wait and
TimedWait don't.

[yhirano, 2014-07-29 06:48:11]

https://codereview.chromium.org/419773002/diff/1/base/synchronization/waitabl...
File base/synchronization/waitable_event_posix.cc (right):

https://codereview.chromium.org/419773002/diff/1/base/synchronization/waitabl...
base/synchronization/waitable_event_posix.cc:293: // We acquire and release the
lock in order to ensure that the
On 2014/07/25 17:32:46, agl wrote:
> It doesn't seem reasonable that WaitMany has this property, but Wait and
> TimedWait don't.

TimedWait has the property because it acquires the lock (see L200 and L202).
Blink WebSocket relies on the property. Now it uses TimedWait and I don't find
any customers that use the property via WaitMany.
Nevertheless, WaitMany with one waitable event should be equivalent to |Wait|, I
think.

In any case, the comment for WaitMany, 

  // You MUST NOT delete any of the WaitableEvent objects while this  wait is
  // happening.

is wrong. We can't delete any of the WaitableEvent objects even when this wait
finishes.
If we don't modify the implementation, we should fix the comment.

[agl, 2014-07-29 21:39:50]

https://codereview.chromium.org/419773002/diff/1/base/synchronization/waitabl...
File base/synchronization/waitable_event_posix.cc (right):

https://codereview.chromium.org/419773002/diff/1/base/synchronization/waitabl...
base/synchronization/waitable_event_posix.cc:293: // We acquire and release the
lock in order to ensure that the
On 2014/07/29 06:48:11, yhirano wrote:
> TimedWait has the property because it acquires the lock (see L200 and L202).

Ah, you have found a bug in the code! TimedWait calls Release even when the
SyncWaiter fired (which is the common case). That call will always fail in that
case, but the return value is ignored so we never noticed.

> Blink WebSocket relies on the property.

The only property that an event provides is that the Wait() call return happens
after the Signal() call. I don't think that I'm being crazy believing that. If
you know of code that assumes more than that, then it's only working because of
the above bug. Ideally it would be fixed and then we could fix that bug.

>   // You MUST NOT delete any of the WaitableEvent objects while this  wait is
>   // happening.
> 
> is wrong. We can't delete any of the WaitableEvent objects even when this wait
> finishes.

Yes you can: the WaitMany has stopped using them. The fact that you have other
threads using them is why you can't delete them.

[yhirano, 2014-07-30 11:43:45]

https://codereview.chromium.org/419773002/diff/1/base/synchronization/waitabl...
File base/synchronization/waitable_event_posix.cc (right):

https://codereview.chromium.org/419773002/diff/1/base/synchronization/waitabl...
base/synchronization/waitable_event_posix.cc:293: // We acquire and release the
lock in order to ensure that the
On 2014/07/29 21:39:49, agl wrote:
> On 2014/07/29 06:48:11, yhirano wrote:
> > TimedWait has the property because it acquires the lock (see L200 and L202).
> 
> Ah, you have found a bug in the code! TimedWait calls Release even when the
> SyncWaiter fired (which is the common case). That call will always fail in
that
> case, but the return value is ignored so we never noticed.
> 
> > Blink WebSocket relies on the property.
> 
> The only property that an event provides is that the Wait() call return
happens
> after the Signal() call. I don't think that I'm being crazy believing that. If
> you know of code that assumes more than that, then it's only working because
of
> the above bug. Ideally it would be fixed and then we could fix that bug.
> 
Hmm... I can easily find multiple code relying on the property ([1] and [2] for
example).
Your opinion is that the following code is wrong, is that right?

{
  WaitableEvent e;
  PostTask(..., &e);
  e.Wait();
}

1:
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...
2:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr...
(CompletionEvent contains a WaitableEvent)

[agl, 2014-07-30 16:40:53]

On 2014/07/30 11:43:45, yhirano wrote:
> Hmm... I can easily find multiple code relying on the property ([1] and [2]
for
> example).
> Your opinion is that the following code is wrong, is that right?
> 
> {
>   WaitableEvent e;
>   PostTask(..., &e);
>   e.Wait();
> }

Yes that's "buggy" based on the intended behaviour of WaitableEvent. The
property of an event is that Wait's return "happens after" Signal is called. I
believe that this is the same as, say, pthread_cond_broadcast's specification.

What you want is that Wait's return "happens after" Signal's return. On a
practical basis, if you believe that this bug has wormed its way into the code
base sufficiently that we can't exercise it then such is the reality of
programming. In that case, I'll document and test the bug and we'll have to live
with it.

And then, if we're going to enshrine the bug then, yep, WaitMany should behave
the same.

So, if you want to assert that we have to enshrine this then I'll do so and send
a CL back to you.

[yhirano, 2014-07-31 11:48:13]

How about taking the lock in the destructor?
As base::Lock asserts that it must not be locked on destruction, the current
working code must not be locked at that point.
It doesn't affect Signal & Wait performance.

With this modification you can remove the "bug" safely, I think.

[yhirano, 2014-07-31 11:48:20]

https://codereview.chromium.org/419773002/diff/20001/base/synchronization/wai...
File base/synchronization/waitable_event_posix.cc (right):

https://codereview.chromium.org/419773002/diff/20001/base/synchronization/wai...
base/synchronization/waitable_event_posix.cc:45: // TODO(yhirano): Write comment
here
I will add a comment tomorrow...

[agl, 2014-08-01 01:00:07]

Although I think that taking a lock in the destructor should work, I think your
original patch was a smaller change and more clearly safe.

I propose https://codereview.chromium.org/430363004

[yhirano, 2014-08-04 08:57:04]

Thank you, I copied your comments.

[agl, 2014-08-04 18:01:33]

lgtm

[yhirano, 2014-08-05 00:53:01]

+willchan for OWNER review.

[willchan no longer on Chromium, 2014-08-05 16:47:22]

I'm looking right now, but the first thing is, do we have a test for this?

[willchan no longer on Chromium, 2014-08-05 16:57:40]

I don't understand why [1] and [2] violate agl's assertion that Wait()'s return
happens after Signal() completes. In those examples, the only thing happening on
the non-Wait()-invoking thread is a call to the base::WaitableEvent::Signal().

[willchan no longer on Chromium, 2014-08-05 17:26:27]

agl@ walked by and explained the difference between the weaker and stronger
invariant, the subtlety of which I didn't fully grasp before. Now it makes
sense.

Once there's a test for this code, I'm happy with it.

https://codereview.chromium.org/419773002/diff/40001/base/synchronization/wai...
File base/synchronization/waitable_event_posix.cc (right):

https://codereview.chromium.org/419773002/diff/40001/base/synchronization/wai...
base/synchronization/waitable_event_posix.cc:298: // By taking this lock here we
ensure that |Signal| has completed by the
Non-obvious on a first read that Signal() is holding this lock. Please add that
explanation.

[yhirano, 2014-08-06 11:49:49]

https://codereview.chromium.org/419773002/diff/40001/base/synchronization/wai...
File base/synchronization/waitable_event_posix.cc (right):

https://codereview.chromium.org/419773002/diff/40001/base/synchronization/wai...
base/synchronization/waitable_event_posix.cc:298: // By taking this lock here we
ensure that |Signal| has completed by the
On 2014/08/05 17:26:27, willchan wrote:
> Non-obvious on a first read that Signal() is holding this lock. Please add
that
> explanation.

Done.

[willchan no longer on Chromium, 2014-08-06 20:39:35]

lgtm

[yhirano, 2014-08-07 01:10:22]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-08-07 01:12:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/419773002/180001

[commit-bot: I haz the power, 2014-08-07 08:09:16]

Change committed as 288005