From 68028317c1d3d831a24f90e2b25d1410ce045c54 Mon Sep 17 00:00:00 2001
From: Klaus Aehlig <aehlig@google.com>
Date: Fri, 9 Jun 2017 14:03:19 -0400
Subject: [PATCH] experimental UI: move stopUpdateThread() out of synchronized,
 again

The experimental UI uses a thread to regularly update itself to reflect time-based
changes. As that that thread has to call synchronized methods any waiting for it to
finish has to happen outside any synchronized block. Unfortunately, 9e0308e0f7 accidentally
moved the stopUpdateThread() in buildComplete() into the synchronized block, thus giving
an opportunity for deadlocks. Move it out again.

Also, as the accounting for pending transports now happens in synchronized methods in
the state tracker, the buildEventTransportClosed() method does not have to be synchronized
any more---thus eliminating the second deadlock opportunity.

Change-Id: Icacb2ee70f4bedde1c1aac2bcfefc6fabf42fdd3
PiperOrigin-RevId: 158537844
---
 .../lib/runtime/ExperimentalEventHandler.java     | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java
index 549b1c34167669..91e67e4454c8dc 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/ExperimentalEventHandler.java
@@ -446,6 +446,7 @@ public void progressReceiverAvailable(ExecutionProgressReceiverAvailableEvent ev
   public void buildComplete(BuildCompleteEvent event) {
     // The final progress bar will flow into the scroll-back buffer, to if treat
     // it as an event and add a timestamp, if events are supposed to have a timestmap.
+    boolean done = false;
     synchronized (this) {
       stateTracker.buildComplete(event);
       ignoreRefreshLimitOnce();
@@ -455,10 +456,13 @@ public void buildComplete(BuildCompleteEvent event) {
       // upload happening.
       if (stateTracker.pendingTransports() == 0) {
         buildComplete = true;
-        stopUpdateThread();
-        flushStdOutStdErrBuffers();
+        done = true;
       }
     }
+    if (done) {
+      stopUpdateThread();
+      flushStdOutStdErrBuffers();
+    }
   }
 
   @Subscribe
@@ -574,7 +578,7 @@ public synchronized void buildEventTransportsAnnounced(AnnounceBuildEventTranspo
   }
 
   @Subscribe
-  public synchronized void buildEventTransportClosed(BuildEventTransportClosedEvent event) {
+  public void buildEventTransportClosed(BuildEventTransportClosedEvent event) {
     stateTracker.buildEventTransportClosed(event);
     if (debugAllEvents) {
       this.handle(Event.info(null, "Transport " + event.transport().name() + " closed"));
@@ -721,6 +725,11 @@ public void run() {
     }
   }
 
+  /**
+   * Stop the update thread and wait for it to terminate. As the update thread, which is a separate
+   * thread, might have to call a synchronized method between being interrupted and terminating, DO
+   * NOT CALL from a SYNCHRONIZED block, as this will give the opportunity for dead locks.
+   */
   private void stopUpdateThread() {
     Thread threadToWaitFor = null;
     synchronized (this) {