Use a more robust strategy for handling deserializing NestedSet interrupts in NestedSetBuilder.
https://github.com/bazelbuild/bazel/commit/4f661fe0e103ac31f23d68b31ba4d4fe716aab0b improved the situation by blocking for futures when adding transitive members to the builder (increasing the chances that the interrupt would be observed prior to calling build). However, we have since learned that even a done future will throw InterruptedException, so we are still seeing this code path triggering JVM halts.
Instead, offer a build() alternative buildInterruptibly() that propagates the interrupt and use this from CppCompileAction.
RELNOTES: None.
PiperOrigin-RevId: 293212806
diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java
index 10b64f3..0e683a4 100644
--- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java
+++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSet.java
@@ -79,7 +79,9 @@
this.memo = LEAF_MEMO;
}
- NestedSet(Order order, Set<E> direct, Set<NestedSet<E>> transitive) {
+ NestedSet(
+ Order order, Set<E> direct, Set<NestedSet<E>> transitive, InterruptStrategy interruptStrategy)
+ throws InterruptedException {
this.orderAndSize = order.ordinal();
// The iteration order of these collections is the order in which we add the items.
@@ -131,7 +133,7 @@
CompactHashSet<E> hoisted = null;
for (NestedSet<E> subset : transitiveOrder) {
// If this is a deserialization future, this call blocks.
- Object c = subset.getChildren();
+ Object c = subset.getChildrenInternal(interruptStrategy);
if (c instanceof Object[]) {
Object[] a = (Object[]) c;
if (a.length < 2) {
@@ -217,7 +219,7 @@
/**
* What to do when an interruption occurs while getting the result of a deserialization future.
*/
- private enum InterruptStrategy {
+ enum InterruptStrategy {
/** Crash with {@link ExitCode#INTERRUPTED}. */
CRASH,
/** Throw {@link InterruptedException}. */
diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetBuilder.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetBuilder.java
index cb27452..12129a9 100644
--- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetBuilder.java
@@ -20,9 +20,8 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.MapMaker;
-import com.google.common.util.concurrent.ListenableFuture;
import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet;
-import com.google.devtools.build.lib.concurrent.MoreFutures;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet.InterruptStrategy;
import com.google.errorprone.annotations.DoNotCall;
import java.util.concurrent.ConcurrentMap;
@@ -150,33 +149,32 @@
}
/**
- * Similar to {@link #addTransitive} except that if the subset is based on a deserialization
- * future, blocks for that future to complete.
- *
- * <p>The block would occur anyway upon calling {@link #build}. However, {@link #build} crashes
- * instead of propagating {@link InterruptedException}. This method may be preferable if the
- * caller can propagate {@link InterruptedException}.
- */
- // TODO(b/146789490): Remove this workaround.
- public NestedSetBuilder<E> addTransitiveAndBlockIfFuture(NestedSet<? extends E> subset)
- throws InterruptedException {
- Object children = subset.rawChildren();
- if (children instanceof ListenableFuture) {
- MoreFutures.waitForFutureAndGet((ListenableFuture<?>) children);
- }
- return addTransitive(subset);
- }
-
- /**
* Builds the actual nested set.
*
* <p>This method may be called multiple times with interleaved {@link #add}, {@link #addAll} and
* {@link #addTransitive} calls.
*/
+ public NestedSet<E> build() {
+ try {
+ return buildInternal(InterruptStrategy.CRASH);
+ } catch (InterruptedException e) {
+ throw new IllegalStateException("Cannot throw with InterruptStrategy.CRASH", e);
+ }
+ }
+
+ /**
+ * Similar to {@link #build} except that if any subset is based on a deserialization future and an
+ * interrupt is observed, {@link InterruptedException} is propagated.
+ */
+ public NestedSet<E> buildInterruptibly() throws InterruptedException {
+ return buildInternal(InterruptStrategy.PROPAGATE);
+ }
+
// Casting from CompactHashSet<NestedSet<? extends E>> to CompactHashSet<NestedSet<E>> by way of
// CompactHashSet<?>.
@SuppressWarnings("unchecked")
- public NestedSet<E> build() {
+ private NestedSet<E> buildInternal(InterruptStrategy interruptStrategy)
+ throws InterruptedException {
if (isEmpty()) {
return order.emptySet();
}
@@ -195,7 +193,8 @@
return new NestedSet<>(
order,
items == null ? ImmutableSet.of() : items,
- transitiveSetsCast == null ? ImmutableSet.of() : transitiveSetsCast);
+ transitiveSetsCast == null ? ImmutableSet.of() : transitiveSetsCast,
+ interruptStrategy);
}
private static final ConcurrentMap<ImmutableList<?>, NestedSet<?>> immutableListCache =
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index aa8950e..3c92fe2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -648,11 +648,11 @@
NestedSetBuilder<Artifact> discoveredModulesBuilder = NestedSetBuilder.stableOrder();
for (Artifact module : topLevel) {
topLevelModulesBuilder.add(module);
- discoveredModulesBuilder.addTransitiveAndBlockIfFuture(transitivelyUsedModules.get(module));
+ discoveredModulesBuilder.addTransitive(transitivelyUsedModules.get(module));
}
topLevelModules = topLevelModulesBuilder.build();
discoveredModulesBuilder.addTransitive(topLevelModules);
- NestedSet<Artifact> discoveredModules = discoveredModulesBuilder.build();
+ NestedSet<Artifact> discoveredModules = discoveredModulesBuilder.buildInterruptibly();
additionalInputs =
NestedSetBuilder.fromNestedSet(additionalInputs).addTransitive(discoveredModules).build();
diff --git a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetImplTest.java b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetImplTest.java
index 62d97fb..2c235d3 100644
--- a/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetImplTest.java
+++ b/src/test/java/com/google/devtools/build/lib/collect/nestedset/NestedSetImplTest.java
@@ -21,7 +21,6 @@
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.SettableFuture;
-import java.util.concurrent.atomic.AtomicBoolean;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -260,27 +259,13 @@
}
@Test
- public void addTransitiveAndBlockIfFuture_propagatesInterrupt() throws Exception {
- SettableFuture<Object[]> deserializationFuture = SettableFuture.create();
+ public void buildInterruptibly_propagatesInterrupt() {
NestedSet<String> deserialzingNestedSet =
- NestedSet.withFuture(Order.STABLE_ORDER, deserializationFuture);
- NestedSetBuilder<String> builder = NestedSetBuilder.stableOrder();
- AtomicBoolean interruptPropagated = new AtomicBoolean();
-
- Thread add =
- new Thread(
- () -> {
- try {
- builder.addTransitiveAndBlockIfFuture(deserialzingNestedSet);
- } catch (InterruptedException e) {
- interruptPropagated.set(true);
- }
- });
- add.start();
- add.interrupt();
- add.join();
-
- assertThat(interruptPropagated.get()).isTrue();
+ NestedSet.withFuture(Order.STABLE_ORDER, SettableFuture.create());
+ NestedSetBuilder<String> builder =
+ NestedSetBuilder.<String>stableOrder().addTransitive(deserialzingNestedSet).add("a");
+ Thread.currentThread().interrupt();
+ assertThrows(InterruptedException.class, builder::buildInterruptibly);
}
@Test
