Skip to content

Fix test_pthread_dlopen_many - #19459

Merged
sbc100 merged 1 commit into
mainfrom
fix_test_pthread_dlopen_many
May 26, 2023
Merged

Fix test_pthread_dlopen_many#19459
sbc100 merged 1 commit into
mainfrom
fix_test_pthread_dlopen_many

Conversation

@sbc100

@sbc100 sbc100 commented May 26, 2023

Copy link
Copy Markdown
Collaborator

When a thread sync was cancelled (due to a thread exit prior to performing the
task) we were treating that as an error, but we should treat that as success.

I was able to reproduce the issue by running this test a few times in
a row. I confirmed that it did not happen in 160 runs after this fix.

Fixes: #18887

@sbc100
sbc100 requested review from dschuff and tlively May 26, 2023 21:50
@dschuff

dschuff commented May 26, 2023

Copy link
Copy Markdown
Member

Can you expand the commit message a little bit to say what was going wrong and how PR fixes it?

@sbc100
sbc100 force-pushed the fix_test_pthread_dlopen_many branch from d6cd944 to 4fcc48f Compare May 26, 2023 23:06

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Where in the test does a cancel occur, though? I can't seem to find that.

When a thread sync was cancelled (due to a thread exit prior to
performing the task) we were treating that as an error, but we should
treat that as success.

I was able to reproduce the issue by running this test a few times in
a row.  I confirmed that it did not happen in 160 runs after this fix.

Fixes: #18887
@sbc100
sbc100 force-pushed the fix_test_pthread_dlopen_many branch from 4fcc48f to d069aa8 Compare May 26, 2023 23:08

@dschuff dschuff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks for the explanation, this makes sense.

@sbc100

sbc100 commented May 26, 2023

Copy link
Copy Markdown
Collaborator Author

Makes sense. Where in the test does a cancel occur, though? I can't seem to find that.

I add some comments and elaborated in the PR description.

The test just start a lot of threads that don't do much and just exit when they are done so its not surprising that we see a thread exit when another one is loading.

@sbc100
sbc100 merged commit 5c27e79 into main May 26, 2023
@sbc100
sbc100 deleted the fix_test_pthread_dlopen_many branch May 26, 2023 23:36
@kripken

kripken commented May 26, 2023

Copy link
Copy Markdown
Member

Hmm, but how can a thread exit before it finishes the task? That's what I'm not seeing in that test. It looks like they each do some straightforward work, so I'd expect them to all finish normally, despite their large number.

@dschuff

dschuff commented May 26, 2023

Copy link
Copy Markdown
Member

I think it's not that they aren't finishing (their own task) normally, it's that they are finishing their own task and exiting before they finish the task of loading all the dlls that were opened on other threads while they were running. In that case the task of loading more dlls is cancelled because the thread is done. But that was being reported as a failure.

@kripken

kripken commented May 27, 2023

Copy link
Copy Markdown
Member

Oh, I see, thanks! Makes sense to me now.

@sbc100

sbc100 commented May 27, 2023

Copy link
Copy Markdown
Collaborator Author

Hmm, but how can a thread exit before it finishes the task? That's what I'm not seeing in that test. It looks like they each do some straightforward work, so I'd expect them to all finish normally, despite their large number.

Basically we dispatch "dlsync" requests to all threads.. But if a thread is just about to exit, the exit might happen before the thread performs the task. In that case it doesn't matter than the thread is not in sync because is no longer running. All threads sync up on startup anyway.

@tlively

tlively commented Jun 5, 2023

Copy link
Copy Markdown
Member

@sbc100, if you want, you could also simplify this code by using emscripten_proxy_promise instead of emscripten_proxy_callback.

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.

test_pthread_dlopen_many is very flaky

4 participants