Fix a deadlock in RemotePathChecker when using a disk cache
#28416
Conversation
fmeum
force-pushed
the
28408-disk-cache-test
branch
from
c65fb50 to
2a83435
Compare
fmeum
changed the title
RemotePathChecker when using a disk cache
|
@bazel-io fork 9.0.1 |
github-actions
bot
added
team-Remote-Exec
|
@bazel-io fork 8.6.0 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses a potential deadlock in RemotePathChecker by refactoring blocking calls into a non-blocking, future-based approach. The changes in RemoteActionFileSystem.java and RemoteExecutionCache.java correctly implement this asynchronous pattern, particularly by updating the RemotePathChecker interface and its usage. The added test parameters in BuildWithoutTheBytesIntegrationTest.java ensure comprehensive testing of the fix with and without disk cache. Overall, the changes are well-implemented and directly resolve the identified issue, improving the system's robustness.
|
I will investigate the test failure. |
fmeum
force-pushed
the
28408-disk-cache-test
branch
from
2a83435 to
bb27d9c
Compare
|
Fixed! |
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the deadlock issue in RemotePathChecker by transitioning from blocking I/O operations to non-blocking ListenableFutures. The changes in RemoteActionFileSystem and RemoteExecutionCache correctly refactor the downloadIfRemote and isRemote methods, respectively, to align with this asynchronous pattern. The renaming of isRemote to isAvailableLocally in RemotePathChecker also enhances clarity regarding its functionality. Additionally, the updated integration tests for BuildWithoutTheBytesIntegrationTest appropriately incorporate disk cache scenarios, improving test coverage for the fix. The overall approach is sound and resolves the described problem.
|
@bazel-io fork 9.1.0 |
coeuvre
added
awaiting-PR-merge
github-actions
bot
removed
the
awaiting-PR-merge
…uild#28416) The call to `RemoteActionFileSystem#getInputStream` in `RemotePathChecker#isRemote` could result in a deadlock since it is executed on the same executor that is also used to prefetch the file from the disk cache. This is fixed by replacing the blocking call with a future. Along the way the method is renamed to avoid confusion due to two different meanings of "remote" (not being available in the disk cache vs. not being materialized at the exec path). Fixing this bug makes it possible to run all BwoB tests with disk cache. Related to bazelbuild#28408 Closes bazelbuild#28416. PiperOrigin-RevId: 862135346 Change-Id: Ic6a0b190be8f4769788078afe481496996f82013
…uild#28416) The call to `RemoteActionFileSystem#getInputStream` in `RemotePathChecker#isRemote` could result in a deadlock since it is executed on the same executor that is also used to prefetch the file from the disk cache. This is fixed by replacing the blocking call with a future. Along the way the method is renamed to avoid confusion due to two different meanings of "remote" (not being available in the disk cache vs. not being materialized at the exec path). Fixing this bug makes it possible to run all BwoB tests with disk cache. Related to bazelbuild#28408 Closes bazelbuild#28416. PiperOrigin-RevId: 862135346 Change-Id: Ic6a0b190be8f4769788078afe481496996f82013
…#28416) (#28465) The call to `RemoteActionFileSystem#getInputStream` in `RemotePathChecker#isRemote` could result in a deadlock since it is executed on the same executor that is also used to prefetch the file from the disk cache. This is fixed by replacing the blocking call with a future. Along the way the method is renamed to avoid confusion due to two different meanings of "remote" (not being available in the disk cache vs. not being materialized at the exec path). Fixing this bug makes it possible to run all BwoB tests with disk cache. Related to #28408 Closes #28416. PiperOrigin-RevId: 862135346 Change-Id: Ic6a0b190be8f4769788078afe481496996f82013 Commit 55e4aac Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
3 participants
The call to
RemoteActionFileSystem#getInputStreaminRemotePathChecker#isRemotecould result in a deadlock since it is executed on the same executor that is also used to prefetch the file from the disk cache.This is fixed by replacing the blocking call with a future. Along the way the method is renamed to avoid confusion due to two different meanings of "remote" (not being available in the disk cache vs. not being materialized at the exec path).
Fixing this bug makes it possible to run all BwoB tests with disk cache.
Related to #28408