From 4b092cdc34e81ac4d4337f549a96a022d6ff7cce Mon Sep 17 00:00:00 2001
From: lpino <lpino@google.com>
Date: Fri, 26 Apr 2019 07:20:53 -0700
Subject: [PATCH] Restore atomicity of sequence number generation and addition
 to the event queue in the BES uploader.

The BES backends expect the sequence number and the order in the queue to coincide and the atomicity was removed in https://github.com/bazelbuild/bazel/commit/7c7957607642b4dead24c8c1fceae214eeb8ff37 so we're restoring it now with this change.

Fixes https://github.com/bazelbuild/bazel/issues/8160

PiperOrigin-RevId: 245416462
---
 .../BuildEventServiceUploader.java            | 43 +++++++++++++------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceUploader.java b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceUploader.java
index 53eab936ccb421..e3c8e805aadfca 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceUploader.java
+++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceUploader.java
@@ -173,19 +173,28 @@ void enqueueEvent(BuildEvent event) {
       }
       return;
     }
-    // BuildCompletingEvent marks the end of the build in the BEP event stream.
-    if (event instanceof BuildCompletingEvent) {
-      synchronized (lock) {
+
+    // The generation of the sequence number and the addition to the {@link #eventQueue} should be
+    // atomic since BES expects the events in that exact order.
+    // More details can be found in b/131393380.
+    // TODO(bazel-team): Consider relaxing this invariant by having a more relaxed order.
+    synchronized (lock) {
+      // BuildCompletingEvent marks the end of the build in the BEP event stream.
+      if (event instanceof BuildCompletingEvent) {
         this.buildStatus = extractBuildStatus((BuildCompletingEvent) event);
       }
+      ensureUploadThreadStarted();
+
+      // TODO(b/131393380): {@link #nextSeqNum} doesn't need to be an AtomicInteger if it's
+      //  always used under lock. It would be cleaner and more performant to update the sequence
+      //  number when we take the item off the queue.
+      eventQueue.addLast(
+          new SendRegularBuildEventCommand(
+              event,
+              localFileUploadFuture,
+              nextSeqNum.getAndIncrement(),
+              Timestamps.fromMillis(clock.currentTimeMillis())));
     }
-    ensureUploadThreadStarted();
-    eventQueue.addLast(
-        new SendRegularBuildEventCommand(
-            event,
-            localFileUploadFuture,
-            nextSeqNum.getAndIncrement(),
-            Timestamps.fromMillis(clock.currentTimeMillis())));
   }
 
   /**
@@ -201,8 +210,18 @@ public ListenableFuture<Void> close() {
 
     ensureUploadThreadStarted();
 
-    // Enqueue the last event which will terminate the upload.
-    eventQueue.addLast(new SendLastBuildEventCommand(nextSeqNum.getAndIncrement(), currentTime()));
+    // The generation of the sequence number and the addition to the {@link #eventQueue} should be
+    // atomic since BES expects the events in that exact order.
+    // More details can be found in b/131393380.
+    // TODO(bazel-team): Consider relaxing this invariant by having a more relaxed order.
+    synchronized (lock) {
+      // Enqueue the last event which will terminate the upload.
+      // TODO(b/131393380): {@link #nextSeqNum} doesn't need to be an AtomicInteger if it's
+      //  always used under lock. It would be cleaner and more performant to update the sequence
+      //  number when we take the item off the queue.
+      eventQueue.addLast(
+          new SendLastBuildEventCommand(nextSeqNum.getAndIncrement(), currentTime()));
+    }
 
     final SettableFuture<Void> finalCloseFuture = closeFuture;
     closeFuture.addListener(