Fixing race between ~AsyncInvoker and ~AsyncClosure.

Fixing race between ~AsyncInvoker and ~AsyncClosure.

The AsyncInvoker destructor waits for all invoked tasks to be complete
(in other words, all AsyncClosures to be destructed). They were using an
event to wake up the destructor, but a race made it possible for this
event to be dereferenced after it's destroyed.

So, instead of using an event, this CL just busy-waits until the tasks
are complete. The advantage of this approach is that the tasks
themselves don't need to do any additional synchronization, which could
harm performance since tasks are invoked much more often than
AsyncInvokers are destroyed.

BUG=webrtc:7656

[Taylor Brandstetter, 2017-05-12 21:57:39]

deadbeef@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, tommi@webrtc.org

[Taylor Brandstetter, 2017-05-12 21:57:39]

Tommi or Karl, what do you think about this? I've tried to fix this twice and
failed; I'm not seeing any way it can be done without introducing an extra
per-invoke lock or busy waiting. So I chose the latter, since it seems like the
lesser evil in this case. But there could be an option I overlooked.

[kwiberg-webrtc, 2017-05-15 08:44:48]

I won't try to review this, since I don't know these classes well enough.
Although if there's no one else, I could take some time and get acquainted.

[Taylor Brandstetter, 2017-05-16 18:44:11]

deadbeef@webrtc.org changed reviewers:
+ nisse@webrtc.org
- kwiberg@webrtc.org

[Taylor Brandstetter, 2017-05-16 18:44:12]

nisse@, maybe you're more familiar with AsyncInvoker and have a suggestion? In
summary: I'm trying to fix this race condition in a way that doesn't introduce
additional locking for every async-invoked task.

[nisse-webrtc, 2017-05-17 07:29:11]

On 2017/05/16 18:44:12, Taylor Brandstetter wrote:
> nisse@, maybe you're more familiar with AsyncInvoker and have a suggestion? In
> summary: I'm trying to fix this race condition in a way that doesn't introduce
> additional locking for every async-invoked task.

I'm not very familiar with this code, and I'm not sure I understand the race 
involving the event.

Using a reference count seems right, problems is how to wait until it gets to
zero. Are 
there any standard patterns for how to do that?

One possibility might be to give the count an initial value of 1, increment it
when each AsyncClosure is created, and decrement it in AsyncClosure destructor.
And in addition, decrement it at the start of the AsyncInvoker destructor.

With that convention, both ~AsyncClosure and ~AsyncInvoker will do an atomic
decrement of the *same* counter, and by checking the return value, they can know
reliably which one is last, which should make it easier to do the right thing.

~AsyncClosure:

 int refs = AtomicOps::Decrement(&invoker_->ref_count);
 if (refs == 0) {
   /* We're last, signal the destructor. */
   invoker_->invocation_complete_.Set();
 }

~AsyncInvoker:

 destroying_ = true;
 int refs = AtomicOps::Decrement(&ref_count);
 if (refs == 0) {
   /* We're last, do nothing */
 } else {
   invocation_complete_.Wait();
   RTC_CHECK(AtomicOps::AcquireLoad(&ref_count) == 0);
 }

Would that work? 

The above sketch makes the assumption that once the refcount goes down to zero,
it will never ever be incremented.

One thing I don't understand is the use of the |destroying_| member variable.
It's not protected in any way, does that mean that the destructor and the
DoInvoke methods are always called on the same thread?
It must be the caller's responsibility to not introduce races between the
destructor and other method calls on the AsyncInvoker object, right? (Since by
definition, destroying the object means that any later, or simultaneous, call to
any method is undefined behaviour).

[Taylor Brandstetter, 2017-05-17 19:58:19]

The race occurs if one thread is in between these two lines while the
AsyncInvoker destructor runs on another thread:

AtomicOps::Decrement(&invoker_->pending_invocations_);
invoker_->invocation_complete_.Set();


Your idea is good, but we also need to handle the situation where an
async-invoked task calls AsyncInvoke again, potentially on the thread that the
AsyncInvoker destructor is being run on. For example:

1. Async-invoked function on thread B is in the middle of calling AsyncInvoke
again; about to add to thread A's message queue.
2. AsyncInvoker destructor runs on Thread A. Sees ref count is nonzero, so
starts waiting.
3. AsyncClosure queued on thread A.
4. Its "parent" AsyncClosure is destroyed, but it's not the last one, so it
doesn't signal anything.
5. Thread A is now stuck waiting.

And to answer your other question, the |destroying_| member variable is just
used to ensure that when async-invoked tasks call AsyncInvoke again, the cycle
is guaranteed to end *eventually*.

Anyway, here's a CL that uses the reference counting approach I was thinking of,
for comparison: https://codereview.webrtc.org/2885143005

[nisse-webrtc, 2017-05-19 07:17:00]

https://codereview.webrtc.org/2876273002/diff/1/webrtc/base/asyncinvoker.cc
File webrtc/base/asyncinvoker.cc (right):

https://codereview.webrtc.org/2876273002/diff/1/webrtc/base/asyncinvoker.cc#n...
webrtc/base/asyncinvoker.cc:29: rtc::Thread::Current()->SleepMs(0);
I think it's better with some small but non-zero value, say 10ms, so we don't
hog the cpu if the pending tasks is sleeping.

https://codereview.webrtc.org/2876273002/diff/1/webrtc/base/asyncinvoker.cc#n...
webrtc/base/asyncinvoker.cc:33: MessageQueueManager::Clear(this);
According to the comment, we only need to clear the queue for the current
thread. Then

  rtc::Thread::Current()->Clear(this);

should be enough, right? The MessageQueueManager method iterates over all
MessageQueues it knows about.

https://codereview.webrtc.org/2876273002/diff/1/webrtc/base/asyncinvoker.cc#n...
webrtc/base/asyncinvoker.cc:68: if (destroying_) {
I'm still a bit confused by this flag. My current understanding is that it's set
in the destructor, and we want it to "eventually" propagate to other threads
that might be posting tasks to the thread that is blocking in the destructor.
Maybe a volatile declaration is good enough for that purpose.

But since it is accessed from multiple threads, tsan is likely to warn about it,
and it would be appropriate using atomic access, and a comment on the weak
synchronization intended.

It's a bit silly, in that I think we already have all the locks needed to do
hard synchronization, but they're private to other classes. What we'd need is a
a Post method with an additional std::function<bool()>  argument, with the
semantics that MessageQueue::Post will call the function *while holding the
message queue lock*, and push the message only if the function returns true.

And a similar callback for the Clear function. Then we could get setting and
checking of the |destroying_| flag to be atomic with respect to the queue
modifications.

I don't have that much experience with thread programming. Are there any known
patterns with that kind of callbacks, to piggy back on another class' atomic
operations? I imagine there are some pitfalls.

[Taylor Brandstetter, 2017-08-04 00:09:23]

Description was changed from

==========
Fixing race between ~AsyncInvoker and ~AsyncClosure.

The AsyncInvoker destructor waits for all invoked tasks to be complete
(in other words, all AsyncClosures to be destructed). They were using an
event to wake up the destructor, but a race made it possible for this
event to be dereferenced after it's destroyed.

So, instead of using an event, this CL just busy-waits until the tasks
are complete. The advantage of this approach is that the tasks
themselves don't need to do any additional synchronization, which could
harm performance since tasks are invoked much more often than
AsyncInvokers are destroyed.

BUG=webrtc:7656
==========

to

==========
Fixing race between ~AsyncInvoker and ~AsyncClosure.

The AsyncInvoker destructor waits for all invoked tasks to be complete
(in other words, all AsyncClosures to be destructed). They were using an
event to wake up the destructor, but a race made it possible for this
event to be dereferenced after it's destroyed.

So, instead of using an event, this CL just busy-waits until the tasks
are complete. The advantage of this approach is that the tasks
themselves don't need to do any additional synchronization, which could
harm performance since tasks are invoked much more often than
AsyncInvokers are destroyed.

BUG=webrtc:7656
==========

[Taylor Brandstetter, 2017-08-04 00:09:33]

Closing this in favor of https://codereview.webrtc.org/2885143005/