-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.1] Enforce and await cleanup in StarlarkBaseExternalContext
#22814
Conversation
`StarlarkBaseExternalContext` now implements `AutoCloseable` and, in `close()`: 1. Cancels all pending async tasks. 2. Awaits their termination. 3. Cleans up the working directory (always for module extensions, on failure for repo rules). 4. Fails if there were pending async tasks in an otherwise successful evaluation. Previously, module extensions didn't do any of those. Repo rules did 1 and 4 and sometimes 3, but not in all cases. This change required replacing the fixed-size thread pool in `DownloadManager` with virtual threads, thereby resolving a TODO about not using a fixed-size thread pool for the `GrpcRemoteDownloader`. Work towards bazelbuild#22680 Work towards bazelbuild#22748 Closes bazelbuild#22772. PiperOrigin-RevId: 644669599 Change-Id: Ib71e5bf346830b92277ac2bd473e11c834cb2624
Hmm, not sure why this is failing:
|
StarlarkBaseExternalContext
StarlarkBaseExternalContext
@meteorcloudy Could you trigger a rerun? It does pass locally for me. |
I already did, but it didn't seem to help. Maybe RBE has some weird java toolchain setup? |
/cc @hvadehra in case you know something about this. |
Head branch was pushed to by a user without write access
(I should have clarified the fix from back then is apparently already in 7.x thanks to 4362d97, but bumping it further could work. If it doesn't, I can take a stab at debugging) |
I also don't mind disabling this test on RBE since we already have it covered on other platforms. |
@@ -72,7 +72,7 @@ function test_jdeps() { | |||
# src/test/shell/bazel/jdeps_class_denylist.txt. | |||
find . -type f -iname \*class | \ | |||
grep -vFf "$denylist" | \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding a sort -r
after this should workaround the problem (for now). I'll look into a principled fix later.
Head branch was pushed to by a user without write access
8fb4dce
[7.2.1] Enforce and await cleanup in `StarlarkBaseExternalContext` (bazelbuild#22814)
…azelbuild#22814) `StarlarkBaseExternalContext` now implements `AutoCloseable` and, in `close()`: 1. Cancels all pending async tasks. 2. Awaits their termination. 3. Cleans up the working directory (always for module extensions, on failure for repo rules). 4. Fails if there were pending async tasks in an otherwise successful evaluation. Previously, module extensions didn't do any of those. Repo rules did 1 and 4 and sometimes 3, but not in all cases. This change required replacing the fixed-size thread pool in `DownloadManager` with virtual threads, thereby resolving a TODO about not using a fixed-size thread pool for the `GrpcRemoteDownloader`. Work towards bazelbuild#22680 Work towards bazelbuild#22748 Closes bazelbuild#22772 PiperOrigin-RevId: 644669599 Change-Id: Ib71e5bf346830b92277ac2bd473e11c834cb2624 Closes bazelbuild#22775
…azelbuild#22814) `StarlarkBaseExternalContext` now implements `AutoCloseable` and, in `close()`: 1. Cancels all pending async tasks. 2. Awaits their termination. 3. Cleans up the working directory (always for module extensions, on failure for repo rules). 4. Fails if there were pending async tasks in an otherwise successful evaluation. Previously, module extensions didn't do any of those. Repo rules did 1 and 4 and sometimes 3, but not in all cases. This change required replacing the fixed-size thread pool in `DownloadManager` with virtual threads, thereby resolving a TODO about not using a fixed-size thread pool for the `GrpcRemoteDownloader`. Work towards bazelbuild#22680 Work towards bazelbuild#22748 Closes bazelbuild#22772 PiperOrigin-RevId: 644669599 Change-Id: Ib71e5bf346830b92277ac2bd473e11c834cb2624 Closes bazelbuild#22775
…azelbuild#22814) `StarlarkBaseExternalContext` now implements `AutoCloseable` and, in `close()`: 1. Cancels all pending async tasks. 2. Awaits their termination. 3. Cleans up the working directory (always for module extensions, on failure for repo rules). 4. Fails if there were pending async tasks in an otherwise successful evaluation. Previously, module extensions didn't do any of those. Repo rules did 1 and 4 and sometimes 3, but not in all cases. This change required replacing the fixed-size thread pool in `DownloadManager` with virtual threads, thereby resolving a TODO about not using a fixed-size thread pool for the `GrpcRemoteDownloader`. Work towards bazelbuild#22680 Work towards bazelbuild#22748 Closes bazelbuild#22772 PiperOrigin-RevId: 644669599 Change-Id: Ib71e5bf346830b92277ac2bd473e11c834cb2624 Closes bazelbuild#22775
StarlarkBaseExternalContext
now implementsAutoCloseable
and, inclose()
:Previously, module extensions didn't do any of those. Repo rules did 1 and 4 and sometimes 3, but not in all cases.
This change required replacing the fixed-size thread pool in
DownloadManager
with virtual threads, thereby resolving a TODO about not using a fixed-size thread pool for theGrpcRemoteDownloader
.Work towards #22680
Work towards #22748
Closes #22772
PiperOrigin-RevId: 644669599
Change-Id: Ib71e5bf346830b92277ac2bd473e11c834cb2624
Closes #22775