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
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 53eab93..e3c8e80 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 @@
}
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 @@
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(
