[7.2.0] Fix certain deadlocks in repo fetching with worker threads #22261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
See code comment. PiperOrigin-RevId: 620977564 Change-Id: I076a58a6ac8890940d086d0120be6a72ba56f53d
PiperOrigin-RevId: 613829935 Change-Id: Ic83cad02a6ad02e6b188395b83f9651da03f7482
I managed to reproduce some deadlocks during repo fetching with virtual worker threads. One notable trigger was some _other_ repo failing to fetch, which seems to cause Skyframe to try to interrupt other concurrent repo fetches. This _might_ be the cause for a deadlock where we submit a task to the worker executor service, but the task never starts running before it gets cancelled, which causes us to wait forever for a `DONE` signal that never comes. (The worker task puts a `DONE` signal in the queue in a `finally` block -- but we don't even enter the `try`.) This PR improves the situation in various ways: 1. Instead of using a `SynchronousQueue` for the signal queue, we now use a Semaphore for signaling. Semaphores have the crucial property that releasing a permit (ie. incrementing the counter) does not block, and thus cannot be interrupted. This means that the worker thread can now reliably send signals the host thread, even when it's interrupted. 2. Instead of using two signals for `DONE` and `RESTART`, we just use the one semaphore for both signals, and rely on `workerFuture.isDone()` to tell whether the worker has finished or is waiting for a fresh Environment. 3. Instead of signaling `DONE` in a `finally` block, we now use a `ListenableFuture` and signal to the semaphore in the worker future's listener. This makes sure that the signaling is performed _after_ the worker future's status changes, and safeguards against the case where the submitted task never starts running before it gets cancelled. 4. Instead of waiting for a `DONE` signal (or, in the new setup, the signal semaphore) to make sure the worker thread has finished, we now hold on to the executor service, which offers a `close()` method that essentially uninterruptibly waits for any scheduled tasks to terminate, whether or not they have started running. (@justinhorvitz had suggested a similar idea before.) To make sure distinct repo fetches don't interfere with each other, we start a separate worker executor service for each repo fetch instead of making everyone share the same worker executor service. (This is recommended for virtual threads; see https://docs.oracle.com/en/java/javase/21/core/virtual-threads.html#GUID-C0FEE349-D998-4C9D-B032-E01D06BE55F2 for example.) And because I now create a separate worker executor service for each repo fetch, it doesn't really make sense to use this for platform threads anymore. So setting `--experimental_worker_for_repo_fetching` to any but `off` will cause us to use virtual threads. Related: #22003 Fixes #21712. Closes #22100. PiperOrigin-RevId: 630534733 Change-Id: If989bf9cae76abb1579a2b1de896df8e5a63b88d
github-actions
bot
added
awaiting-review
iancha1992
approved these changes
May 6, 2024
github-actions
bot
removed
the
awaiting-review
keertk
pushed a commit
that referenced
this pull request
Jun 10, 2024
Release Notes: Configurability: + aquery: `//foo:bar` now means "all configured targets with label `//foo:bar`" instead of "choose an arbitrary configured target with label `//foo:bar`". This is in line with cquery behavior. (#22135) + Added a new flag `--incompatible_disable_native_repo_rules` to disable native repo rule usage in WORKSPACE. All native repo rules now have a Starlark counterpart that can be used in both WORKSPACE and Bzlmod; see #22080 for more details. (#22203) + Starlark command-line flags can now be referred to through `alias` targets. (#22212) ExternalDeps: + bzlmod `git_repository` now accepts the `strip_prefix` arg and passes it to the underlying `git_repository` call. (#22137) + Added a new `include()` directive to `MODULE.bazel` files, which allows the root module file to be divided into multiple segments. (#22204) + Fixed certain deadlocks in repo fetching with worker threads (`--experimental_worker_for_repo_fetching=auto`). (#22261) + `print` statements in module files are now only executed for the root module and modules subject to non-registry overrides (e.g. `local_path_override`). (#22263) + The new `refresh` value for `--lockfile_mode` behaves like the `update` mode, but additionally forces a refresh of mutable registry content (yanked versions and missing module versions) when switched to or from time to time while enabled. (#22371) + `Label` instances passed to `print` or `fail` as positional arguments are now formatted with apparent repository names (optimized for human readability). (#22460) + Changes to environment variables read via `getenv` now correctly invalidate module extensions. (#22541) + Git merge conflicts in `MODULE.bazel.lock` files can be resolved automatically. See https://bazel.build/external/lockfile#automatic-resolution for the required setup. (#22650) OSS: + Bazel on Linux and BSD now respects the XDG_CACHE_HOME environment variable instead of assuming that ~/.cache/bazel is writable. (#21817) Performance: + Paths in the Linux sandbox are now again identical to those outside the sandbox, even with `--incompatible_sandbox_hermetic_tmp`. (#22407) Remote-Exec: + The combined coverage report produced via `--combined_report=lcov` is now announced on the BES via the new `CoverageReport` event. (#22327) + The compact and full execution logs now contain start times for spawns (if available). (#22341) Rules-CPP: + The default Unix C++ toolchain now supports the `parse_headers` feature to validate header files with `--process_headers_in_dependencies`. (#22369) Starlark-Interpreter: + Starlark `min` and `max` buitins now allow a `key` callback, similarly to `sorted`. (#21960) Acknowledgements: This release contains contributions from many people at Google, as well as bazel.build machine account, Brentley Jones, Cameron Martin, Daniel Wagner-Hall, Douglas Thor, Fabian Meumertzheim, George Gensure, hvd, Isaac Torres, Keith Smiley, Mark Elliot, oquenchil, Romain Chossart, Son Luong Ngoc, Spencer Putt, Thomas Weischuh, Xdng Yng, Xùdōng Yáng, Zheng Wei Tan.
copybara-service bot
pushed a commit
that referenced
this pull request
Jun 10, 2024
Release Notes: Configurability: + aquery: `//foo:bar` now means "all configured targets with label `//foo:bar`" instead of "choose an arbitrary configured target with label `//foo:bar`". This is in line with cquery behavior. (#22135) + Added a new flag `--incompatible_disable_native_repo_rules` to disable native repo rule usage in WORKSPACE. All native repo rules now have a Starlark counterpart that can be used in both WORKSPACE and Bzlmod; see #22080 for more details. (#22203) + Starlark command-line flags can now be referred to through `alias` targets. (#22212) ExternalDeps: + bzlmod `git_repository` now accepts the `strip_prefix` arg and passes it to the underlying `git_repository` call. (#22137) + Added a new `include()` directive to `MODULE.bazel` files, which allows the root module file to be divided into multiple segments. (#22204) + Fixed certain deadlocks in repo fetching with worker threads (`--experimental_worker_for_repo_fetching=auto`). (#22261) + `print` statements in module files are now only executed for the root module and modules subject to non-registry overrides (e.g. `local_path_override`). (#22263) + The new `refresh` value for `--lockfile_mode` behaves like the `update` mode, but additionally forces a refresh of mutable registry content (yanked versions and missing module versions) when switched to or from time to time while enabled. (#22371) + `Label` instances passed to `print` or `fail` as positional arguments are now formatted with apparent repository names (optimized for human readability). (#22460) + Changes to environment variables read via `getenv` now correctly invalidate module extensions. (#22541) + Git merge conflicts in `MODULE.bazel.lock` files can be resolved automatically. See https://bazel.build/external/lockfile#automatic-resolution for the required setup. (#22650) OSS: + Bazel on Linux and BSD now respects the XDG_CACHE_HOME environment variable instead of assuming that ~/.cache/bazel is writable. (#21817) Performance: + Paths in the Linux sandbox are now again identical to those outside the sandbox, even with `--incompatible_sandbox_hermetic_tmp`. (#22407) Remote-Exec: + The combined coverage report produced via `--combined_report=lcov` is now announced on the BES via the new `CoverageReport` event. (#22327) + The compact and full execution logs now contain start times for spawns (if available). (#22341) Rules-CPP: + The default Unix C++ toolchain now supports the `parse_headers` feature to validate header files with `--process_headers_in_dependencies`. (#22369) Starlark-Interpreter: + Starlark `min` and `max` buitins now allow a `key` callback, similarly to `sorted`. (#21960) Acknowledgements: This release contains contributions from many people at Google, as well as bazel.build machine account, Brentley Jones, Cameron Martin, Daniel Wagner-Hall, Douglas Thor, Fabian Meumertzheim, George Gensure, hvd, Isaac Torres, Keith Smiley, Mark Elliot, oquenchil, Romain Chossart, Son Luong Ngoc, Spencer Putt, Thomas Weischuh, Xdng Yng, Xùdōng Yáng, Zheng Wei Tan.
rdeushane
pushed a commit
to SibrosTech/bazel
that referenced
this pull request
Jun 19, 2024
Release Notes: Configurability: + aquery: `//foo:bar` now means "all configured targets with label `//foo:bar`" instead of "choose an arbitrary configured target with label `//foo:bar`". This is in line with cquery behavior. (bazelbuild#22135) + Added a new flag `--incompatible_disable_native_repo_rules` to disable native repo rule usage in WORKSPACE. All native repo rules now have a Starlark counterpart that can be used in both WORKSPACE and Bzlmod; see bazelbuild#22080 for more details. (bazelbuild#22203) + Starlark command-line flags can now be referred to through `alias` targets. (bazelbuild#22212) ExternalDeps: + bzlmod `git_repository` now accepts the `strip_prefix` arg and passes it to the underlying `git_repository` call. (bazelbuild#22137) + Added a new `include()` directive to `MODULE.bazel` files, which allows the root module file to be divided into multiple segments. (bazelbuild#22204) + Fixed certain deadlocks in repo fetching with worker threads (`--experimental_worker_for_repo_fetching=auto`). (bazelbuild#22261) + `print` statements in module files are now only executed for the root module and modules subject to non-registry overrides (e.g. `local_path_override`). (bazelbuild#22263) + The new `refresh` value for `--lockfile_mode` behaves like the `update` mode, but additionally forces a refresh of mutable registry content (yanked versions and missing module versions) when switched to or from time to time while enabled. (bazelbuild#22371) + `Label` instances passed to `print` or `fail` as positional arguments are now formatted with apparent repository names (optimized for human readability). (bazelbuild#22460) + Changes to environment variables read via `getenv` now correctly invalidate module extensions. (bazelbuild#22541) + Git merge conflicts in `MODULE.bazel.lock` files can be resolved automatically. See https://bazel.build/external/lockfile#automatic-resolution for the required setup. (bazelbuild#22650) OSS: + Bazel on Linux and BSD now respects the XDG_CACHE_HOME environment variable instead of assuming that ~/.cache/bazel is writable. (bazelbuild#21817) Performance: + Paths in the Linux sandbox are now again identical to those outside the sandbox, even with `--incompatible_sandbox_hermetic_tmp`. (bazelbuild#22407) Remote-Exec: + The combined coverage report produced via `--combined_report=lcov` is now announced on the BES via the new `CoverageReport` event. (bazelbuild#22327) + The compact and full execution logs now contain start times for spawns (if available). (bazelbuild#22341) Rules-CPP: + The default Unix C++ toolchain now supports the `parse_headers` feature to validate header files with `--process_headers_in_dependencies`. (bazelbuild#22369) Starlark-Interpreter: + Starlark `min` and `max` buitins now allow a `key` callback, similarly to `sorted`. (bazelbuild#21960) Acknowledgements: This release contains contributions from many people at Google, as well as bazel.build machine account, Brentley Jones, Cameron Martin, Daniel Wagner-Hall, Douglas Thor, Fabian Meumertzheim, George Gensure, hvd, Isaac Torres, Keith Smiley, Mark Elliot, oquenchil, Romain Chossart, Son Luong Ngoc, Spencer Putt, Thomas Weischuh, Xdng Yng, Xùdōng Yáng, Zheng Wei Tan.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RELNOTES: Fixed certain deadlocks in repo fetching with worker threads (
--experimental_worker_for_repo_fetching=auto).