Skip to content

Commit

Permalink
IPC: Fix flaky IPC unit test
Browse files Browse the repository at this point in the history
For thread API conformance, a base::Thread must be
started/stopped/destructed on the same thread. For the IPC unit tests,
the IPC thread is created on the "listener" thread, and the CL
https://crrev.com/c/4116950 was created to ensure that the IPC thread
was also stopped/destroyed on the "listener" thread as well.

However, that CL introduced a race condition. The flow is like so:
  1. Main thread attempts to shut down, but must block on the listener
     thread, so it posts a task to the listener thread to finish up and
     notify a sync wait event (for the listener thread) that the main
     thread "waits" on
  2. Listener thread must destroy the IPC thread, so it creates a sync
     event and posts a task to the IPC thread to finish up and notify
     the sync event
  3. IPC thread finishes itself up, notifies the IPC event (that the
     listener thread is waiting for) and then posts another task back to
     the listener thread to notify the event (that the main thread is
     waiting on) that it is finished
  4. Back on the listener thread, which is now ublocked by the IPC
     waitable event, the IPC thread is `reset()`/destructed

This means it is possible for the main thread to get notified about the
listener thread shutdown, at the same time the listener thread is
attempting to `reset()`/destroy the IPC thread, which is a race.

To fix this, we have to serialize the operations. The new flow is:
  1. Main thread attempts to shut down, and blocks on the listener
     thread's waitable event (just like before)
  2. Listener thread creates an IPC waitable event, and posts a task to
     the IPC thread to finish up and notify the IPC waitable event
  3. Only when the IPC thread is finished, does the listener thread
     destroy the IPC thread and notify the main thread that the listener
     thread is finally done

This gives us the guarantee that when the listener event is finally
unblocked, and the main thread continues destruction, the IPC
base::Thread has already been `reset()`/destroyed, and there is no race
in destruction.

R=rockot@google.com

Bug: 1411770
Change-Id: I8f98cbf62a78ea363c6c67dc17ed424ee33cde05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4210470
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/main@{#1099554}
  • Loading branch information
domfarolino authored and Chromium LUCI CQ committed Feb 1, 2023
1 parent 46de447 commit 3ff421c
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions ipc/ipc_sync_channel_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -210,16 +210,16 @@ class Worker : public Listener, public Sender {
ipc_event.Wait();
// This destructs `ipc_thread_` on the listener thread.
ipc_thread_.reset();

listener_thread_.task_runner()->PostTask(
FROM_HERE, base::BindOnce(&Worker::OnListenerThreadShutdown2,
base::Unretained(this), listener_event));
}

void OnIPCThreadShutdown(WaitableEvent* listener_event,
WaitableEvent* ipc_event) {
base::RunLoop().RunUntilIdle();
ipc_event->Signal();

listener_thread_.task_runner()->PostTask(
FROM_HERE, base::BindOnce(&Worker::OnListenerThreadShutdown2,
base::Unretained(this), listener_event));
}

void OnListenerThreadShutdown2(WaitableEvent* listener_event) {
Expand Down

0 comments on commit 3ff421c

Please sign in to comment.