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

fix: fix flaky tests and non blocking semaphore #1365

merged 3 commits into from May 4, 2021


Copy link

@mutianf mutianf commented May 4, 2021

Fix for #1359

In NonBlockingSemaphore, updating both limit and avaliablePermits in increase and decrease limits may cause problems. If the limit is lowered but avaliablePermits is not updated yet, and release is called, this statement in release: if (availablePermits.compareAndSet(old, Math.min(old + permits, limit.get()))) may update the availablePermits incorrectly.

Since we're already tracking the limit in the semaphore class, instead of tracking what's available, we could track how many permits are acquired. So When we're adjusting the limit, we won't need to update both variables and should avoid this problem.

After the change the test succeeded in all 100 runs:

//                     PASSED in 1.7s
  Stats over 100 runs: max = 1.7s, min = 0.9s, avg = 1.2s, dev = 0.1s

@mutianf mutianf requested review from as code owners May 4, 2021
@google-cla google-cla bot added the cla: yes label May 4, 2021
Copy link

@vam-google vam-google left a comment

LGTM, but can you please confirm that it is still green on 1,000 and maybe 10,000 runs?

Copy link
Contributor Author

@mutianf mutianf commented May 4, 2021

10000 runs with flag

all passed

INFO: 10001 processes: 1 internal, 10000 linux-sandbox.
INFO: Build completed successfully, 10001 total actions
//                     PASSED in 2.4s
  Stats over 10000 runs: max = 2.4s, min = 0.9s, avg = 1.3s, dev = 0.2s

@vam-google vam-google merged commit fc8e520 into googleapis:master May 4, 2021
7 checks passed
@mutianf mutianf deleted the fix_flaky_test branch Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cla: yes
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants