Skip to content

Commit

Permalink
Keep track of markerData correctly when using worker threads for repo…
Browse files Browse the repository at this point in the history
… fetching.

Previously, the first markerData map was passed into the worker thread, then, on a Skyframe restart, RepositoryFunction would create a new one which was left empty.

Work towards #10515 .

RELNOTES: None.
PiperOrigin-RevId: 553704893
Change-Id: I57fd03d478f727ec7d38beb09da8148150e71fa3
  • Loading branch information
lberki authored and copybara-github committed Aug 4, 2023
1 parent 896b358 commit 4627227
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState;
import java.util.Map;
import java.util.TreeMap;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.Future;
import java.util.concurrent.SynchronousQueue;
Expand Down Expand Up @@ -51,11 +53,13 @@ enum Signal {

/** The channel for the worker thread to send a signal to the host Skyframe thread. */
final BlockingQueue<Signal> signalQueue = new SynchronousQueue<>();

/**
* The channel for the host Skyframe thread to send fresh {@link SkyFunction.Environment} objects
* back to the worker thread.
*/
final BlockingQueue<SkyFunction.Environment> delegateEnvQueue = new SynchronousQueue<>();

/**
* This future holds on to the worker thread in order to cancel it when necessary; it also serves
* to tell whether a worker thread is already running.
Expand All @@ -66,6 +70,14 @@ enum Signal {
// we might block in `close()`, which is potentially bad (see its javadoc).
@Nullable volatile Future<RepositoryDirectoryValue.Builder> workerFuture = null;

/**
* This is where the {@code markerData} for the whole invocation is collected.
*
* <p>{@link com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction} creates a
* new map on each restart, so we can't simply plumb that in.
*/
final Map<String, String> markerData = new TreeMap<>();

SkyFunction.Environment signalForFreshEnv() throws InterruptedException {
signalQueue.put(Signal.RESTART);
return delegateEnvQueue.take();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ protected boolean verifySemanticsMarkerData(Map<String, String> markerData, Envi
// false is the safe thing to do.
return false;
}

return describeSemantics(starlarkSemantics).equals(markerData.get(SEMANTICS));
}

Expand Down Expand Up @@ -158,7 +159,7 @@ public RepositoryDirectoryValue.Builder fetch(
() -> {
try {
return fetchInternal(
rule, outputDirectory, directories, workerEnv, markerData, key);
rule, outputDirectory, directories, workerEnv, state.markerData, key);
} finally {
state.signalQueue.put(Signal.DONE);
}
Expand All @@ -174,7 +175,9 @@ public RepositoryDirectoryValue.Builder fetch(
return null;
case DONE:
try {
return workerFuture.get();
RepositoryDirectoryValue.Builder result = workerFuture.get();
markerData.putAll(state.markerData);
return result;
} catch (ExecutionException e) {
Throwables.throwIfInstanceOf(e.getCause(), RepositoryFunctionException.class);
Throwables.throwIfUnchecked(e.getCause());
Expand Down

0 comments on commit 4627227

Please sign in to comment.