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.

Sign up for GitHub

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

Jump to bottom

--bes_timeout is ignored in Bazel 5 #14576

Closed
BalestraPatrick opened this issue Jan 13, 2022 · 6 comments
Closed

--bes_timeout is ignored in Bazel 5 #14576

BalestraPatrick opened this issue Jan 13, 2022 · 6 comments
Assignees
@coeuvre
Labels
P1 I'll work on this now. (Assignee required) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: bug

Comments

@BalestraPatrick
Copy link
Member

Description of the problem / feature request:

--bes_timeout doesn't seem to be respected anymore in Bazel 5.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

This will complete successfully and upload the full BES even though it shouldn't because the timeout is set to a very low 1ms:

$ git clone git@github.com:bazelbuild/rules_apple.git
$ cd rules_apple
$ env USE_BAZEL_VERSION=5.0.0rc4 bazelisk build --remote_cache=grpcs://remote.buildbuddy.io --bes_backend=grpcs://remote.buildbuddy.io --bes_results_url=https://app.buildbuddy.io/invocation/ //examples/iOS/StickersApp --bes_timeout=1ms --cpu=ios_x86_64

While this will fail as expected in Bazel 4.2.1:

$ env USE_BAZEL_VERSION=4.2.1 bazelisk build --remote_cache=grpcs://remote.buildbuddy.io --bes_backend=grpcs://remote.buildbuddy.io --bes_results_url=https://app.buildbuddy.io/invocation/ //examples/iOS/StickersApp --bes_timeout=1ms --cpu=ios_x86_64
ERROR: The Build Event Protocol upload timed out. com.google.common.util.concurrent.TimeoutFuture$TimeoutFutureException: Timed out: com.google.common.util.concurrent.Futures$NonCancellationPropagatingFuture@43553e48[status=PENDING, info=[delegate=[com.google.common.util.concurrent.SettableFuture@4d92c852[status=PENDING]]]]

What operating system are you running Bazel on?

Replace this line with your answer.

macOS 12.1
@brentleyjones
Copy link
Contributor

cc @coeuvre

@sventiffe sventiffe added team-Remote-Exec Issues and PRs for the Execution (Remote) team untriaged labels Jan 14, 2022
@coeuvre coeuvre self-assigned this Jan 19, 2022
@coeuvre coeuvre added P1 I'll work on this now. (Assignee required) type: bug and removed untriaged labels Jan 19, 2022
@coeuvre
Copy link
Member

coeuvre commented Jan 19, 2022

After 4.2, we introduced async upload for remote module and we added code to block waiting unfinished futures at the end of build command. However, this piece of code is executed before relevant code in BES module where the flag --bes_timeout is used at.

I have some quick-and-dirty fixes which I am not satisfied with. I have a long-term fix but it maybe too risky to put into 5.0rc4. WDYT if we fix this in 5.1?

@brentleyjones
Copy link
Contributor

Since 5.0 was just released, whatever the next release is, 5.0.1 or 5.1 sounds good.

@BalestraPatrick
Copy link
Member Author

This is not a blocker for us to adopt Bazel 5.0, so we're fine with a fix in a future version.

@brentleyjones
Copy link
Contributor

@Wyverald its not fixed yet, but once it is, I think this would be good for a 5.0.1.

@coeuvre coeuvre mentioned this issue Jan 21, 2022
Closed
@Wyverald Wyverald added this to the 5.1 release blockers milestone Jan 21, 2022
@lukaciko lukaciko mentioned this issue Jan 21, 2022
Closed
bazel-io pushed a commit that referenced this issue Jan 25, 2022
@coeuvre @copybara-github
Fix that bepTransports are not properly closed if --bes_timeout is set.
73a76a8 
Related #14576.

PiperOrigin-RevId: 424026771
@Wyverald
Copy link
Member

Wyverald commented Feb 3, 2022

@bazel-io fork 5.1

@bazel-io bazel-io mentioned this issue Feb 3, 2022
Closed
@Wyverald Wyverald removed this from the 5.1 release blockers milestone Feb 3, 2022
brentleyjones pushed a commit to brentleyjones/bazel that referenced this issue Feb 16, 2022
@coeuvre @brentleyjones
Remote: Postpone the block waiting in afterCommand to `BlockWaiting…
6ceb939 
…Module`

When implementing async upload, we introduced a block waiting behaviour in `RemoteModule#afterCommand` so that uploads happened in the background can be waited before the whole build complete.

However, there are other block waiting code in other module's `afterCommand` method (e.g. BES module). Block waiting in remote module will prevent other modules' `afterCommand` from executing until it's completed. This causes issues like bazelbuild#14576.

This PR adds a new module `BlockWaitingModule` whose sole purpose is to accept tasks submitted by other modules in `afterCommand` and block waiting all the tasks in its own `afterCommand` method. So those tasks can be executed in parallel.

This PR only updates RemoteModule's `afterCommand` method to submit block waiting task. Other modules should be updated to use `BlockWaitingModule` as well but that's beyond the scope this this PR.

This PR along with 73a76a8 fix bazelbuild#14576.

Closes bazelbuild#14618.

PiperOrigin-RevId: 424295121
(cherry picked from commit 621649d)
brentleyjones pushed a commit to brentleyjones/bazel that referenced this issue Feb 16, 2022
@coeuvre @brentleyjones
Remote: Postpone the block waiting in afterCommand to `BlockWaiting…
bb67d2e 
…Module`

When implementing async upload, we introduced a block waiting behaviour in `RemoteModule#afterCommand` so that uploads happened in the background can be waited before the whole build complete.

However, there are other block waiting code in other module's `afterCommand` method (e.g. BES module). Block waiting in remote module will prevent other modules' `afterCommand` from executing until it's completed. This causes issues like bazelbuild#14576.

This PR adds a new module `BlockWaitingModule` whose sole purpose is to accept tasks submitted by other modules in `afterCommand` and block waiting all the tasks in its own `afterCommand` method. So those tasks can be executed in parallel.

This PR only updates RemoteModule's `afterCommand` method to submit block waiting task. Other modules should be updated to use `BlockWaitingModule` as well but that's beyond the scope this this PR.

This PR along with 73a76a8 fix bazelbuild#14576.

Closes bazelbuild#14618.

PiperOrigin-RevId: 424295121
(cherry picked from commit 621649d)
@brentleyjones brentleyjones mentioned this issue Feb 16, 2022
Merged
brentleyjones pushed a commit to brentleyjones/bazel that referenced this issue Feb 16, 2022
@coeuvre @brentleyjones
Remote: Postpone the block waiting in afterCommand to `BlockWaiting…
4032fed 
…Module`

When implementing async upload, we introduced a block waiting behaviour in `RemoteModule#afterCommand` so that uploads happened in the background can be waited before the whole build complete.

However, there are other block waiting code in other module's `afterCommand` method (e.g. BES module). Block waiting in remote module will prevent other modules' `afterCommand` from executing until it's completed. This causes issues like bazelbuild#14576.

This PR adds a new module `BlockWaitingModule` whose sole purpose is to accept tasks submitted by other modules in `afterCommand` and block waiting all the tasks in its own `afterCommand` method. So those tasks can be executed in parallel.

This PR only updates RemoteModule's `afterCommand` method to submit block waiting task. Other modules should be updated to use `BlockWaitingModule` as well but that's beyond the scope this this PR.

This PR along with 73a76a8 fix bazelbuild#14576.

Closes bazelbuild#14618.

PiperOrigin-RevId: 424295121
(cherry picked from commit 621649d)
Wyverald pushed a commit that referenced this issue Feb 16, 2022
@brentleyjones @coeuvre
Remote: Postpone the block waiting in afterCommand to `BlockWaiting…
0c74741 
…Module` (#14833)

When implementing async upload, we introduced a block waiting behaviour in `RemoteModule#afterCommand` so that uploads happened in the background can be waited before the whole build complete.

However, there are other block waiting code in other module's `afterCommand` method (e.g. BES module). Block waiting in remote module will prevent other modules' `afterCommand` from executing until it's completed. This causes issues like #14576.

This PR adds a new module `BlockWaitingModule` whose sole purpose is to accept tasks submitted by other modules in `afterCommand` and block waiting all the tasks in its own `afterCommand` method. So those tasks can be executed in parallel.

This PR only updates RemoteModule's `afterCommand` method to submit block waiting task. Other modules should be updated to use `BlockWaitingModule` as well but that's beyond the scope this this PR.

This PR along with 73a76a8 fix #14576.

Closes #14618.

PiperOrigin-RevId: 424295121
(cherry picked from commit 621649d)

Co-authored-by: Chi Wang <chiwang@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@coeuvre coeuvre

Labels
P1 I'll work on this now. (Assignee required) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remote: Postpone the block waiting in afterCommand to BlockWaitingModule coeuvre/bazel
5 participants
@brentleyjones @Wyverald @BalestraPatrick @coeuvre @sventiffe