Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.2.0] Fix a race condition in IncrementalPackageRoots. #22127

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

bazel-io
Copy link
Member

Consider the following scenario:

  • Top level targets A and B
  • To execute actions in A, we need to plant symlinks for NestedSet<Package>: [A1, C1, [D1]],
  • To execute actions in B, we need to plant symlinks for NestedSet<Package>: [B1, C1, [D1]]

In the end, we expect to see symlinks to A1, B1, C1 and D1.

What went wrong

With the current code, there are 2 possible race conditions:

  1. Transitive NestedSet level:
  • Start to plant symlinks for A
  • NestedSet [D1] added to handledPackageNestedSets. No symlink planted yet.
  • Start to plant symlinks for B
  • NestedSet [D1] seen as "handled" and immediately skipped. Planted B1 and C1. B moves on to execution.
    => actions from B that requires D1 would fail (no such file or directory).
  1. Individual symlink level:
  • Start to plant symlinks for A
  • C1 added to lazilyPlantedSymlinks. No symlink planted yet.
  • Start to plant symlinks for B
  • C1 already seen in lazilyPlantedSymlinks and immediately skipped. Planted B1 and D1. B moves on to execution.
    => actions from B that requires C1 would fail (no such file or directory).

The Solution

In order to prevent this race condition, we can plant the symlinks for top level targets A and B sequentially. This gives us the guarantee that: for an action foo under a top level target A, foo is only executed when all the necessary symlinks for A are already planted.

The above scenario would look like:

  • Start to plant symlinks for A
  • The TopLevelTargetReadyForSymlinkPlanting event for B arrived and is held in the sequential event queue
  • Plant all symlinks. lazilyPlantedSymlinks: [A1, C1, D1]. A moves on to execution.
  • Start to plant symlinks for B
  • NestedSet [D1] already seen in handledPackageNestedSets and immediately skipped.
  • C1 already seen in lazilyPlantedSymlinks and immediately skipped.
  • Planted B1. B moves on to execution.

As an (hopefully not premature) optimization, the symlinks under a single top level target are planted in parallel.

Fixes #22073

Verified locally with something similar to the repro in #22073 (comment).

PiperOrigin-RevId: 628080361
Change-Id: Ic6c1a6606d26400c46aa98bfeddc844abd075d0a

Commit 52adf0b

Consider the following scenario:

- Top level targets `A` and `B`
- To execute actions in `A`, we need to plant symlinks for `NestedSet<Package>: [A1, C1, [D1]]`,
- To execute actions in `B`, we need to plant symlinks for `NestedSet<Package>: [B1, C1, [D1]]`

In the end, we expect to see symlinks to `A1`, `B1`, `C1` and `D1`.

**What went wrong**

With the current code, there are 2 possible race conditions:

1. Transitive NestedSet level:
- Start to plant symlinks for `A`
- `NestedSet [D1]` added to `handledPackageNestedSets`. _No symlink planted yet_.
- Start to plant symlinks for `B`
- `NestedSet [D1]` seen as "handled" and immediately skipped. Planted `B1` and `C1`. `B` moves on to execution.
=> actions from `B` that requires `D1` would fail (no such file or directory).

2. Individual symlink level:
- Start to plant symlinks for `A`
- `C1` added to `lazilyPlantedSymlinks`. _No symlink planted yet_.
- Start to plant symlinks for `B`
- `C1` already seen in `lazilyPlantedSymlinks` and immediately skipped. Planted `B1` and `D1`. `B` moves on to execution.
=> actions from `B` that requires `C1` would fail (no such file or directory).

**The Solution**

In order to prevent this race condition, we can plant the symlinks for top level targets `A` and `B` sequentially. This gives us the guarantee that: for an action `foo` under a top level target `A`, `foo` is only executed when all the necessary symlinks for `A` are already planted.

The above scenario would look like:
- Start to plant symlinks for `A`
- The `TopLevelTargetReadyForSymlinkPlanting` event for `B` arrived and is held in the sequential event queue
- Plant all symlinks. `lazilyPlantedSymlinks: [A1, C1, D1]`. `A` moves on to execution.
- Start to plant symlinks for `B`
- `NestedSet [D1]` already seen in `handledPackageNestedSets` and immediately skipped.
- `C1` already seen in `lazilyPlantedSymlinks` and immediately skipped.
- Planted `B1`. `B` moves on to execution.

As an (hopefully not premature) optimization, the symlinks under a single top level target are planted in parallel.

Fixes bazelbuild#22073

Verified locally with something similar to the repro in bazelbuild#22073 (comment).

PiperOrigin-RevId: 628080361
Change-Id: Ic6c1a6606d26400c46aa98bfeddc844abd075d0a
@bazel-io bazel-io requested a review from a team as a code owner April 25, 2024 15:15
@bazel-io bazel-io added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc awaiting-review PR is awaiting review from an assigned reviewer labels Apr 25, 2024
@iancha1992 iancha1992 requested a review from joeleba April 25, 2024 17:37
@iancha1992 iancha1992 enabled auto-merge April 25, 2024 17:37
@iancha1992 iancha1992 added this pull request to the merge queue Apr 26, 2024
Merged via the queue into bazelbuild:release-7.2.0 with commit 5734d2a Apr 26, 2024
33 checks passed
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Core Skyframe, bazel query, BEP, options parsing, bazelrc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants