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

[6.2.0]Use less subshells and tees in running tests with bazel run. #17869

Merged
merged 2 commits into from
Mar 24, 2023

Conversation

ShreeM01
Copy link
Contributor

@ShreeM01 ShreeM01 commented Mar 23, 2023

Fixes #17754.

What we have seen prior to this change was that sometimes for quick tests the output was swallowed. After a lot of poking it became clear that the culprit is the use of subshell and tee, e.g. if you remove tee completely from the picture the behavior never shows up.

The issue is that with a fast test, tee seems to be killed (or its parent subshell) before the printing the output to stdout.

With this change, we reduce the number of subshells and processes to set up and reduce the chance of the race condition but not remove it.

However, for practical purposes, the race condition is gone.

With the reproduction steps in #17754, and this command

for i in {1..10000}; do /tmp/bazel run :foo &> /tmp/log ; grep -q "useful echo" /tmp/log ; if [ $? -eq 0 ]; then echo -n '+'; else  echo -n '-'; fi; done

a bazel from head fails ~3900 out of 10000 times.

After this commit, it never failed.

Closes #17846.
Commit: c04f0d4
PiperOrigin-RevId: 518794237
Change-Id: I8c1862d3a274799b864f0f5f42b85d6df5af78c7

Fixes bazelbuild#17754.

What we have seen prior to this change was that sometimes for quick tests the output was swallowed. After a lot of poking it became clear that the culprit is the use of subshell and `tee`, e.g. if you remove `tee` completely from the picture the behavior never shows up.

The issue is that with a fast test, `tee` seems to be killed (or its parent subshell) before the printing the output to stdout.

With this change, we reduce the number of subshells and processes to set up and reduce the chance of the race condition but not remove it.

However, for practical purposes, the race condition is gone.

With the reproduction steps in bazelbuild#17754, and this command
```
for i in {1..10000}; do /tmp/bazel run :foo &> /tmp/log ; grep -q "useful echo" /tmp/log ; if [ $? -eq 0 ]; then echo -n '+'; else  echo -n '-'; fi; done
```
a bazel from head fails ~3900 out of 10000 times.

After this commit, it never failed.

Closes bazelbuild#17846.

PiperOrigin-RevId: 518794237
Change-Id: I8c1862d3a274799b864f0f5f42b85d6df5af78c7
@ShreeM01 ShreeM01 added awaiting-review PR is awaiting review from an assigned reviewer team-CLI Console UI labels Mar 23, 2023
@ShreeM01 ShreeM01 requested a review from meisterT March 23, 2023 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer team-CLI Console UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants