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 so we're restoring it now with this change.
Fixes
@@ -173,13 +173,21 @@ public final class BuildEventServiceUploader implements Runnable {
}
return;
}
+
+ // 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) {
- synchronized (lock) {
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,
@@ -187,6 +195,7 @@ public final class BuildEventServiceUploader implements Runnable {
nextSeqNum.getAndIncrement(),
Timestamps.fromMillis(clock.currentTimeMillis())));
}
+ }
/**
* Gracefully stops the BES upload. All events enqueued before the call to close will be uploaded
@@ -201,8 +210,18 @@ public final class BuildEventServiceUploader implements Runnable {
ensureUploadThreadStarted();
+ // 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.
- eventQueue.addLast(new SendLastBuildEventCommand(nextSeqNum.getAndIncrement(), currentTime()));
+ // 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(
