Fix flake in test_pthread_proxying_refcount #25097
Merged
+39
−19
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It apparently takes more than one turn of the worker thread's event loop to ensure that the notifications on the zombie task queues have been cleared so the zombies can be culled. Update the test to wait two turns of the event loop instead of one. Also add new synchronization forcing the main thread to wait until the worker thread has entered Wasm before proxying work to it. This prevents the proxied work notifications from somehow being cleared before the worker thread destroys the proxy queues, which would prevent the task queues from ever being placed on the zombie list in the first place. Finally, generally improve comments and make the test assertions more specific.
|
Very nice, thanks for the attention. Ran this several times on Windows, Linux and macOS, and it does look watertight now. |
juj
approved these changes
Aug 29, 2025
sbc100
reviewed
Aug 29, 2025
| @@ -29,22 +34,28 @@ void __attribute__((noinline)) free(void* ptr) { | |||
|
|
|||
| #endif // SANITIZER | |||
|
|
|||
| _Atomic int worker_started = 0; | |||
| _Atomic int should_execute = 0; | |||
There was a problem hiding this comment.
Can you use bool for these two atomics to make the purpose more clear?
sbc100
approved these changes
Aug 29, 2025
sbc100
reviewed
Aug 29, 2025
|
|
||
| void* execute_and_free_queue(void* arg) { | ||
| // Signal the main thread to proxy work to us. | ||
| worker_started = 1; |
There was a problem hiding this comment.
(I assumed you could use true/false in the all the assignment too).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It apparently takes more than one turn of the worker thread's event loop
to ensure that the notifications on the zombie task queues have been
cleared so the zombies can be culled. Update the test to wait two turns
of the event loop instead of one.
Also add new synchronization forcing the main thread to wait until the
worker thread has entered Wasm before proxying work to it. This prevents
the proxied work notifications from somehow being cleared before the
worker thread destroys the proxy queues, which would prevent the task
queues from ever being placed on the zombie list in the first place.
Finally, generally improve comments and make the test assertions more
specific.
Fixes #19795.