Making AsyncInvoker destructor thread-safe.

Making AsyncInvoker destructor thread-safe.

The documentation for AsyncInvoker states that it owns the lifetime of
calls, and when its destructor is called, all in-flight calls are
cancelled or finish executing. The "cancelled" part is working, but if
a call is in the middle of executing, the destructor does *not* wait.

This is fixed by keeping a count of pending invocations, which is
decremented when a call is either cleared from a message queue or
finishes executing.

BUG=webrtc:3914, webrtc:3911
Review-Url: https://codereview.webrtc.org/2694723004
Cr-Commit-Position: refs/heads/master@{#16811}
Committed: https://chromium.googlesource.com/external/webrtc/+/162cb53e7b6b892ef5201c5ce242520694bab9c1

[Taylor Brandstetter, 2017-02-14 04:04:50]

The CQ bit was checked by deadbeef@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 04:04:57]

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

[Taylor Brandstetter, 2017-02-14 04:05:48]

deadbeef@webrtc.org changed reviewers:
+ kjellander@webrtc.org, pthatcher@webrtc.org

[Taylor Brandstetter, 2017-02-14 04:05:49]

pthatcher: PTAL
kjellander: need your lgtm for removed TSan suppressions

[commit-bot: I haz the power, 2017-02-14 04:35:03]

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

[commit-bot: I haz the power, 2017-02-14 04:35:05]

Dry run: Try jobs failed on following builders:
  win_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_dbg/builds/16180)

[Taylor Brandstetter, 2017-02-14 05:15:50]

The CQ bit was checked by deadbeef@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 05:15:59]

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

[commit-bot: I haz the power, 2017-02-14 06:33:39]

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

[commit-bot: I haz the power, 2017-02-14 06:33:39]

Dry run: This issue passed the CQ dry run.

[kjellander_webrtc, 2017-02-14 06:40:57]

tools-webrtc/sanitizers/tsan_suppressions_webrtc.cc: lgtm

Awesome work fixing this old sourdough (Swedish saying for something that's been
around stinking for a long time).

[magjed_webrtc, 2017-02-15 12:19:15]

Description was changed from

==========
Making AsyncInvoker destructor thread-safe.

The documentation for AsyncInvoker states that it owns the lifetime of
calls, and when its destructor is called, all in-flight calls are
cancelled or finish executing. The "cancelled" part is working, but if
a call is in the middle of executing, the destructor does *not* wait.

This is fixed by keeping a count of pending invocations, which is
decremented when a call is either cleared from a message queue or
finishes executing.

BUG=webrtc:3914, webrtc:3911
==========

to

==========
Making AsyncInvoker destructor thread-safe.

The documentation for AsyncInvoker states that it owns the lifetime of
calls, and when its destructor is called, all in-flight calls are
cancelled or finish executing. The "cancelled" part is working, but if
a call is in the middle of executing, the destructor does *not* wait.

This is fixed by keeping a count of pending invocations, which is
decremented when a call is either cleared from a message queue or
finishes executing.

BUG=webrtc:3914, webrtc:3911, webrtc:7146
==========

[magjed_webrtc, 2017-02-15 12:20:25]

Description was changed from

==========
Making AsyncInvoker destructor thread-safe.

The documentation for AsyncInvoker states that it owns the lifetime of
calls, and when its destructor is called, all in-flight calls are
cancelled or finish executing. The "cancelled" part is working, but if
a call is in the middle of executing, the destructor does *not* wait.

This is fixed by keeping a count of pending invocations, which is
decremented when a call is either cleared from a message queue or
finishes executing.

BUG=webrtc:3914, webrtc:3911, webrtc:7146
==========

to

==========
Making AsyncInvoker destructor thread-safe.

The documentation for AsyncInvoker states that it owns the lifetime of
calls, and when its destructor is called, all in-flight calls are
cancelled or finish executing. The "cancelled" part is working, but if
a call is in the middle of executing, the destructor does *not* wait.

This is fixed by keeping a count of pending invocations, which is
decremented when a call is either cleared from a message queue or
finishes executing.

BUG=webrtc:3914, webrtc:3911
==========

[Taylor Brandstetter, 2017-02-16 01:58:23]

deadbeef@webrtc.org changed reviewers:
- kjellander@webrtc.org

[Taylor Brandstetter, 2017-02-16 01:58:23]

Ping

[pthatcher1, 2017-02-17 18:19:37]

pthatcher@webrtc.org changed reviewers:
+ tommi@webrtc.org

[pthatcher1, 2017-02-17 18:19:38]

I think tommi should review this code as well.  I know he was focused on
concurrency code previously.

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc
File webrtc/base/asyncinvoker.cc (right):

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker....
webrtc/base/asyncinvoker.cc:90: AtomicOps::Increment(&pending_invocations_);
Do we have any sense of what perf impact these additional atomic operations and
events firing will have?  We these methods are called a lot in our code (maybe
several times per packet?).

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker....
webrtc/base/asyncinvoker.cc:120: invoker_->invocation_complete_.Set();
Why does it need the friend class to touch a private member?  Why not just have
a public method that can be called?

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/thread_unitte...
File webrtc/base/thread_unittest.cc (right):

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/thread_unitte...
webrtc/base/thread_unittest.cc:561: functor_finished.Wait(Event::kForever);
Nice unit test

[Taylor Brandstetter, 2017-02-17 19:00:46]

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc
File webrtc/base/asyncinvoker.cc (right):

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker....
webrtc/base/asyncinvoker.cc:90: AtomicOps::Increment(&pending_invocations_);
On 2017/02/17 18:19:38, pthatcher1 wrote:
> Do we have any sense of what perf impact these additional atomic operations
and
> events firing will have?  We these methods are called a lot in our code (maybe
> several times per packet?).

Assuming the CPU architecture supports atomic operations, should be pretty low;
much lower than the cost of a mutex. See
https://www.arangodb.com/2015/02/comparing-atomic-mutex-rwlocks/ and
http://stackoverflow.com/questions/2538070/atomic-operation-cost

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker....
webrtc/base/asyncinvoker.cc:120: invoker_->invocation_complete_.Set();
On 2017/02/17 18:19:38, pthatcher1 wrote:
> Why does it need the friend class to touch a private member?  Why not just
have
> a public method that can be called?

Because only AsyncClosure should be able to do this, which is exactly the
purpose of friend classes. This is an acceptable use of "friend" since
AsyncClosure is an implementation detail of AsyncInvoker which nothing outside
it will use directly.

[Taylor Brandstetter, 2017-02-22 01:12:44]

tommi: Can you review or suggest another reviewer?

[tommi, 2017-02-22 11:19:53]

lgtm - one question below as I think we might still have a slim chance of
hitting a race, but clearly this is a big improvement.

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc
File webrtc/base/asyncinvoker.cc (right):

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker....
webrtc/base/asyncinvoker.cc:28: while
(AtomicOps::AcquireLoad(&pending_invocations_)) {
is there a chance that we could get 0 returned here, immediately run out of
quanta on the current thread, another thread increments pending_invocations_ (it
would have to have checked |destroying_| first, but I think that's a
possibility) and later we continue with destruction on this thread with a
non-zero pending_invocations_?

[Taylor Brandstetter, 2017-02-23 01:19:00]

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc
File webrtc/base/asyncinvoker.cc (right):

https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker....
webrtc/base/asyncinvoker.cc:28: while
(AtomicOps::AcquireLoad(&pending_invocations_)) {
On 2017/02/22 11:19:53, tommi (webrtc) wrote:
> is there a chance that we could get 0 returned here, immediately run out of
> quanta on the current thread, another thread increments pending_invocations_
(it
> would have to have checked |destroying_| first, but I think that's a
> possibility) and later we continue with destruction on this thread with a
> non-zero pending_invocations_?

That would mean that the invoker is being destroyed at the same time as
AsyncInvoke is being called on another thread, and *not* from within an
AsyncClosure. I don't think there's any way to guarantee that that's
thread-safe. The destructor could finish, and then the other thread could access
destroying_ and segfault.

The thing the "destroying_" flag does protect against is this kind of thing:

invoker.AsyncInvoke(thread_a, [&invoker]{
  Foo();
  invoker.AsyncInvoke(thread_b, []{
    Bar();
  });
});

Suppose after I call this, I destroy the invoker, on the same thread that the
initial AsyncInvoke was called on.

If "Foo()" is in progress, then the destructor will wait for it to complete, the
inner AsyncInvoke will return early due to "destroying_", and we'll be ok.

And even if the destructor is called *right* after the destroying_ flag is
checked, pending_invocations_ will still be 1, because the outer AsyncClosure
hasn't yet finished. So pending_invocations_ will be incremented to 2, then
decremented to 1, and the destructor will either end up waiting for Bar() to
finish, or MessageQueueManager::Clear() will clear the Bar task.

[tommi, 2017-02-23 09:03:40]

On 2017/02/23 01:19:00, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker.cc
> File webrtc/base/asyncinvoker.cc (right):
> 
>
https://codereview.webrtc.org/2694723004/diff/80001/webrtc/base/asyncinvoker....
> webrtc/base/asyncinvoker.cc:28: while
> (AtomicOps::AcquireLoad(&pending_invocations_)) {
> On 2017/02/22 11:19:53, tommi (webrtc) wrote:
> > is there a chance that we could get 0 returned here, immediately run out of
> > quanta on the current thread, another thread increments pending_invocations_
> (it
> > would have to have checked |destroying_| first, but I think that's a
> > possibility) and later we continue with destruction on this thread with a
> > non-zero pending_invocations_?
> 
> That would mean that the invoker is being destroyed at the same time as
> AsyncInvoke is being called on another thread, and *not* from within an
> AsyncClosure. I don't think there's any way to guarantee that that's
> thread-safe. The destructor could finish, and then the other thread could
access
> destroying_ and segfault.
> 
> The thing the "destroying_" flag does protect against is this kind of thing:
> 
> invoker.AsyncInvoke(thread_a, [&invoker]{
>   Foo();
>   invoker.AsyncInvoke(thread_b, []{
>     Bar();
>   });
> });
> 
> Suppose after I call this, I destroy the invoker, on the same thread that the
> initial AsyncInvoke was called on.
> 
> If "Foo()" is in progress, then the destructor will wait for it to complete,
the
> inner AsyncInvoke will return early due to "destroying_", and we'll be ok.
> 
> And even if the destructor is called *right* after the destroying_ flag is
> checked, pending_invocations_ will still be 1, because the outer AsyncClosure
> hasn't yet finished. So pending_invocations_ will be incremented to 2, then
> decremented to 1, and the destructor will either end up waiting for Bar() to
> finish, or MessageQueueManager::Clear() will clear the Bar task.

Thanks for the clarification

[Taylor Brandstetter, 2017-02-23 23:10:34]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2017-02-23 23:10:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2694723004/#ps100001 (title:
"Merge with master")

[commit-bot: I haz the power, 2017-02-23 23:10:42]

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

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

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

[commit-bot: I haz the power, 2017-02-23 23:29:25]

Try jobs failed on following builders:
  linux_tsan2 on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/20154)

[Taylor Brandstetter, 2017-02-24 00:54:12]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2017-02-24 00:54:15]

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

[commit-bot: I haz the power, 2017-02-24 01:10:08]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487897652082030,
"parent_rev": "8e1435519ca58f367ce1774a4927e048dcef57cc", "commit_rev":
"162cb53e7b6b892ef5201c5ce242520694bab9c1"}

[commit-bot: I haz the power, 2017-02-24 01:10:11]

Description was changed from

==========
Making AsyncInvoker destructor thread-safe.

The documentation for AsyncInvoker states that it owns the lifetime of
calls, and when its destructor is called, all in-flight calls are
cancelled or finish executing. The "cancelled" part is working, but if
a call is in the middle of executing, the destructor does *not* wait.

This is fixed by keeping a count of pending invocations, which is
decremented when a call is either cleared from a message queue or
finishes executing.

BUG=webrtc:3914, webrtc:3911
==========

to

==========
Making AsyncInvoker destructor thread-safe.

The documentation for AsyncInvoker states that it owns the lifetime of
calls, and when its destructor is called, all in-flight calls are
cancelled or finish executing. The "cancelled" part is working, but if
a call is in the middle of executing, the destructor does *not* wait.

This is fixed by keeping a count of pending invocations, which is
decremented when a call is either cleared from a message queue or
finishes executing.

BUG=webrtc:3914, webrtc:3911

Review-Url: https://codereview.webrtc.org/2694723004
Cr-Commit-Position: refs/heads/master@{#16811}
Committed:
https://chromium.googlesource.com/external/webrtc/+/162cb53e7b6b892ef5201c5ce...
==========

[commit-bot: I haz the power, 2017-02-24 01:10:12]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/162cb53e7b6b892ef5201c5ce...