Skip to content

[test] Fix flakiness in test_pthread_wait32/64_notify #20412

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

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 7, 2023

Avoid racing between the main thread and the worker by only triggering the main thread to notify once the workers done printing all its messages.

@sbc100 sbc100 requested review from juj, dschuff and brendandahl October 7, 2023 22:08
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 7, 2023

the reason these tests were not flaky on the browser test suite is because the output is not being tested/verified there.

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 7, 2023

The flakes we are seeing look like this BTW:

--- expected
+++ actual
@@ -5,7 +5,8 @@
 Waiting on address with unexpected value should return 'not-equal' also if timeout==0
 Waiting for 0 nanoseconds should return 'timed-out'
 Waiting for >0 nanoseconds should return 'timed-out'
+main: seen worker running
 Waiting for infinitely long should return 'ok'
-main: seen worker running
 main: waking worker
+Test finished

@sbc100 sbc100 force-pushed the fix_flakiness branch 2 times, most recently from 0578e9d to cf1f27e Compare October 9, 2023 22:59
@sbc100 sbc100 requested review from kripken and removed request for dschuff and brendandahl October 9, 2023 23:00
@sbc100 sbc100 force-pushed the fix_flakiness branch 3 times, most recently from 831ca15 to fc1fc47 Compare October 10, 2023 15:44
@@ -66,15 +65,14 @@ void* thread_main(void* arg) {


EM_BOOL main_loop(double time, void *userData) {
if (addr == 1) {
if (addr == 3) {
// Burn one second to make sure worker finishes its test.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR seems like an improvement, but as a side note, this part of the test ("burn one second") looks fishy...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that is pretty nasty.. but its also pre-existing. I looked into trying to fix that but it could be more involved so leaving it for now.

@sbc100 sbc100 enabled auto-merge (squash) October 10, 2023 16:18
Avoid racing between the main thread and the worker by only triggering
the main thread to notify once the workers done printing all its
messages.
@sbc100 sbc100 merged commit 4b33521 into emscripten-core:main Oct 11, 2023
@sbc100 sbc100 deleted the fix_flakiness branch October 11, 2023 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants