Skip to content

Commit

Permalink
Don't use worker threads for repo fetching during Skyframe error bubb…
Browse files Browse the repository at this point in the history
…ling

For some reason, using worker threads for repo fetching during Skyframe error bubbling frequently causes deadlocks on Linux. I wasn't able to find out why the deadlock happens, but this CL is the immediate solution to the problem, and shouldn't be a performance concern since no Skyframe restarts should happen during error bubbling anyway.

Tested on Linux; with this CL, `bazel test //src/test/shell/bazel:starlark_repository_test --test_filter=test_download_failure_message --runs_per_test=20` finishes just fine. (On an M1 macbook, I can't trigger the deadlock even without this CL.)

Fixes #21238

PiperOrigin-RevId: 606305306
Change-Id: I6f47a144b29030011f6c10c2b37f6874190fed0e
  • Loading branch information
Wyverald authored and copybara-github committed Feb 12, 2024
1 parent c796aba commit fb25158
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,14 @@ public RepositoryDirectoryValue.Builder fetch(
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws RepositoryFunctionException, InterruptedException {
if (workerExecutorService == null) {
if (workerExecutorService == null
|| env.inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors()) {
// Don't use the worker thread if we're in Skyframe error bubbling. For some reason, using a
// worker thread during error bubbling very frequently causes deadlocks on Linux platforms.
// The deadlock is rather elusive and this is just the immediate thing that seems to help.
// Fortunately, no Skyframe restarts should happen during error bubbling anyway, so this
// shouldn't be a performance concern. See https://github.com/bazelbuild/bazel/issues/21238
// for more context.
return fetchInternal(rule, outputDirectory, directories, env, recordedInputValues, key);
}
var state = env.getState(RepoFetchingSkyKeyComputeState::new);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ default void injectVersionForNonHermeticFunction(Version version) {}
* may be called upon to transform a lower-level exception. This method can tell it whether to
* transform a dependency's exception or ignore it and return a value as usual.
*/
// TODO: Rename this as it can be called for other purposes.
boolean inErrorBubblingForSkyFunctionsThatCanFullyRecoverFromErrors();

/**
Expand Down

0 comments on commit fb25158

Please sign in to comment.