Conversation
|
|
||
| // Only check the mailbox if we have a live pthread runtime. We implement | ||
| // pthread_self to return 0 if there is no live runtime. | ||
| var pthread_ptr = _pthread_self(); |
There was a problem hiding this comment.
From the comment here it looks like the intention was that this check should take care of that case.
Perhaps at least we don't need both of these checks?
CC @tlively who wrote this code originally and @brendandahl who recent fixed a shutdown race condition.
There was a problem hiding this comment.
pthread_self() is a native function, so it gets a boundary wrapper that aborts if it is attempted to be called without a live runtime.
Checking for _pthread_self() should be also needed in addition to checking for ABORT, since a given Worker might not host a live pthread at any given moment.
There was a problem hiding this comment.
It looks like there is a special callout here for _pthread_self():
emscripten/tools/emscripten.py
Lines 900 to 903 in b132141
Perhaps that TODO can now be fixed and we can remove pthread_self and _emscripten_proxy_execute_task_queue those from install_debug_wrapper?
There was a problem hiding this comment.
That sounds like a separate PR to look into.
There was a problem hiding this comment.
I think removing 'pthread_self' from this list should probably be part of this PR since you've basically made it so that its no longer called after the runtime exits, right?
There was a problem hiding this comment.
Based on fixing this one test, I have no proof that pthread_self() wouldn't be called anywhere else after the runtime exits.
Please let's not string in more investigative work into PRs. If you want to look into whether removing pthread_self() handling there is safe, that's fine, though I don't have that information without further testing, which I don't want to get roped into doing right now.
There was a problem hiding this comment.
One problem I see here is that we are kind of muddying the waters here about that it means to be a live runtime. We have 3 different signals here: pthread_self, ABORT and runtimeExited.
I would love to have just a single condition that we could check here, but maybe we can just add a TODO to do further investigation. For example, I would hope that on on abort or runtime exit then pthread_self would get set to null (since the thread is no longer hosting a pthread). So I would hope these checks are redundant. The fact that they are not redundant seems like a bug worth investigating.
So lgtm with a TODO to cleanup?
There was a problem hiding this comment.
Oh, wait, I just noticed that callUserCallback already contains these checks.
So maybe the issue is only with -sMINIMAL_RUNTIME where callUserCallback is doesn't contain the checks?
In that case I think maybe the correct fix here is to add the ABORT and runtimeExited checks to callUserCallback in libcore.js line 2117.
There was a problem hiding this comment.
(Also I guess the scope of the callUserCallback should be widened to be the whole body of checkMailbox)
There was a problem hiding this comment.
we are kind of muddying the waters here
This muddying has already happened. I do agree with you that boolean farming is not ideal, and having a single source of truth would be great. But this PR does not add anything new here.
So maybe the issue is only with -sMINIMAL_RUNTIME where callUserCallback is doesn't contain the checks?
#20067 does not use MINIMAL_RUNTIME where the issue reproduces.
Also I guess the scope of the callUserCallback should be widened to be the whole body of checkMailbox
I don't think that's it, since there is an Atomics.asyncWait() involved.
What I'll do is I'll shelve this PR, and leave my contribution here to be an updated test to elevate the flaky failure to a more deterministic failure, #25066. Then you can look into the fix when you have time.
| if (runtimeExited) return; | ||
| #endif | ||
| #if !MINIMAL_RUNTIME | ||
| if (ABORT) return; |
There was a problem hiding this comment.
Do we ever need both this checks? i.e. should this be an elif ?
There was a problem hiding this comment.
I don't think so.
Both booleans are used elsewhere in code, so I think it is correct to be symmetric here as well.
5353e71 to
e0d4e11
Compare
7a069dd to
05da967
Compare
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (1) test expectation files were updated by running the tests with `--rebaseline`: ``` code_size/test_codesize_hello_dylink_all.json: 844375 => 844360 [-15 bytes / -0.00%] Average change: -0.00% (-0.00% - -0.00%) ```
…fter_quit # Conflicts: # test/code_size/test_codesize_hello_dylink_all.json # test/code_size/test_codesize_minimal_pthreads.json # test/code_size/test_codesize_minimal_pthreads_memgrowth.json
|
Closing pending a better fix. |
Fix pthread mailbox scanning to not take place after program has quit or crashed. Closes #20067.