Skip to content

Commit

Permalink
Ensure that pending events in BuildEventStreamer are resolved in FIFO…
Browse files Browse the repository at this point in the history
… order.

1. `TestAttempt` events would wait for the `TargetCompleteEvent` to be posted before being posted.

2. There was an implicit requirement for the `TestAttempt` events to be posted in a specific order.

3. This didn't break in the noskymeld case because we fulfilled this ordering by using the order of performing the attempts themselves. The sequence would look like:
    + post `TargetCompleteEvent`
    -> perform attempt #1
    -> post `TestAttempt` #1
    -> perform attempt bazelbuild#2
    -> post `TestAttempt` bazelbuild#2

4. With skymeld, however, it could happen like this:

    + defer `TargetCompleteEvent` to wait for `CoverageActionFinishedEvent`
    + perform attempt #1 -> defer posting `TestAttempt` #1 & wait for `TargetCompleteEvent`
    + perform attempt bazelbuild#2 -> defer posting `TestAttempt` bazelbuild#2 & wait for `TargetCompleteEvent`
    + `CoverageActionFinishedEvent` -> release & post `TargetCompleteEvent`
    + `TargetCompleteEvent` -> release & post `TestAttempt` bazelbuild#2
    + `TargetCompleteEvent` -> release & post `TestAttempt` #1

Due to (2), the undefined ordering in (4) would cause an issue.

This CL fixes that by ensuring a FIFO ordering of the deferred events.

PiperOrigin-RevId: 572165337
Change-Id: Iac4d023d946865b8b81f15b119417192dc4b5c53
  • Loading branch information
joeleba authored and copybara-github committed Oct 10, 2023
1 parent 66ac39c commit 5cdb9e1
Showing 1 changed file with 12 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@
import static com.google.common.base.Strings.nullToEmpty;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Multimap;
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.common.eventbus.AllowConcurrentEvents;
import com.google.common.eventbus.Subscribe;
Expand Down Expand Up @@ -106,8 +106,11 @@ private enum RetentionDecision {
@GuardedBy("this")
private List<Pair<String, String>> bufferedStdoutStderrPairs = new ArrayList<>();

// Use LinkedHashMultimap to maintain a FIFO ordering of pending events.
// This is important in case of Skymeld, so that the TestAttempt events are resolved in the
// correct order.
@GuardedBy("this")
private final Multimap<BuildEventId, BuildEvent> pendingEvents = HashMultimap.create();
private final SetMultimap<BuildEventId, BuildEvent> pendingEvents = LinkedHashMultimap.create();

@GuardedBy("this")
private int progressCount;
Expand Down Expand Up @@ -493,11 +496,11 @@ public void buildEvent(BuildEvent event) {
}

// Reconsider all events blocked by the event just posted.
Collection<BuildEvent> toReconsider;
Set<BuildEvent> blockedEventsFifo;
synchronized (this) {
toReconsider = pendingEvents.removeAll(event.getEventId());
blockedEventsFifo = pendingEvents.removeAll(event.getEventId());
}
for (BuildEvent freedEvent : toReconsider) {
for (BuildEvent freedEvent : blockedEventsFifo) {
buildEvent(freedEvent);
}

Expand Down Expand Up @@ -558,14 +561,14 @@ private void maybePostPendingEventsBeforeDiscarding(BuildEvent event) {
// a lot of extra locking e.g. for every ActionExecutedEvent and it's only necessary to
// check for this where events are configured to "post after" events that may be discarded.
BuildEventId eventId = event.getEventId();
Collection<BuildEvent> toReconsider;
Set<BuildEvent> blockedEventsFifo;
synchronized (this) {
toReconsider = pendingEvents.removeAll(eventId);
blockedEventsFifo = pendingEvents.removeAll(eventId);
// Pretend we posted this event so a target summary arriving after this test summary (which
// is common) doesn't get erroneously buffered in bufferUntilPrerequisitesReceived().
postedEvents.add(eventId);
}
for (BuildEvent freedEvent : toReconsider) {
for (BuildEvent freedEvent : blockedEventsFifo) {
buildEvent(freedEvent);
}
}
Expand Down

0 comments on commit 5cdb9e1

Please sign in to comment.