Fix Asyncify.handleAsync conflict with PROXY_SYNC_ASYNC#26513
Fix Asyncify.handleAsync conflict with PROXY_SYNC_ASYNC#26513thiblahute wants to merge 1 commit intoemscripten-core:mainfrom
Conversation
|
Thanks! Yes, I just saw #26482 landed ppoll support - I've rebased and dropped the ppoll commit. The remaining change here fixes a pre-existing bug in |
thiblahute
changed the title
| #if PTHREADS | ||
| // When called from a proxied function (PROXY_SYNC_ASYNC), the proxy | ||
| // mechanism handles the async return. Skip the Asyncify unwind. | ||
| if (PThread.currentProxiedOperationCallerThread) return startAsync(); |
There was a problem hiding this comment.
I feel like this might be slightly the wrong place for this change.
I wonder if calling code should instead just not be using handleAsync in the case of PROXY_SYNC_ASYNC?
There was a problem hiding this comment.
The thing is handleAsync is auto-generated by jsifier.mjs when __async: 'auto' is set on a library function. The calling code (e.g. __syscall_poll) doesn't invoke handleAsync directly, it gets wrapped automatically by the compiler. So to avoid using handleAsync in the PROXY_SYNC_ASYNC case, I think we'd need to change jsifier.mjs to not generate that wrapper when __proxy: 'sync' is also present, which felt like a bigger change. But maybe that's actually cleaner? What do you think?
There was a problem hiding this comment.
Yes, I think maybe handleAsyncFunction should take proxyingMode generate a check for PThread.currentProxiedOperationCallerThread in that case
Is it always that case that Asyncify.handleAsync is unnecessary in that case? i.e. in both ASYNCIFY=1 and ASYNCIFY=2?
| (end.tv_nsec - begin.tv_nsec) / 1000000; | ||
| assert(elapsed_ms >= 195); | ||
|
|
||
| printf("done\n"); |
There was a problem hiding this comment.
Do we need a new test here or can we just re-used the existing test test_poll_blocking_asyncify.c but with -sPROXY_TO_PTHREAD?
If there are things we are not covering in the existing test maybe better to add to that?
There was a problem hiding this comment.
I tried reusing test_poll_blocking_asyncify.c with -sPROXY_TO_PTHREAD but it hangs because it uses emscripten_set_timeout to schedule a write, which doesn't work when main runs on a worker thread.
The pthread variant needs actual cross thread writes to exercise the proxied poll path, so I think a separate test file is needed here. Or do you have a better idea?
There was a problem hiding this comment.
emscripten_set_timeout doesn't work when run from a pthread?
Is that because the pthread being blocked during poll rather than unwinding to the even loop? I guess the SYNC_ASYNC proxying more is taking precidence of asyncify in this case and poll is not causing and unwind?
thiblahute
force-pushed
the
ppoll_proxy
branch
5 times, most recently
from
9127310 to
5089fe8
Compare
When in a proxied context, skip the Asyncify unwind and call startAsync() directly, letting the proxy mechanism handle the async return. This already affects __syscall_poll and would also affect the new __syscall_ppoll.
| return NULL; | ||
| } | ||
|
|
||
| int main(void) { |
There was a problem hiding this comment.
This code looks a lot like the existing test_unblock_poll + write_after_sleep code in test_poll_blocking.c.
Would it be enough to run test_poll_blocking.c in PROXY_TO_PTHREAD+ASYNCIFY mode? (rather than add this new test code)
| assert(ppoll(&pfd, 1, &ts, NULL) == 0); | ||
| clock_gettime(CLOCK_MONOTONIC, &end); | ||
| long elapsed_ms = (end.tv_sec - begin.tv_sec) * 1000 + | ||
| (end.tv_nsec - begin.tv_nsec) / 1000000; |
There was a problem hiding this comment.
The other test files in this family use the timespec_delta_ms helper for this.
2 participants
Fix Asyncify.handleAsync conflict with PROXY_SYNC_ASYNC
When in a proxied context, skip the Asyncify unwind and call
startAsync() directly, letting the proxy mechanism handle the
async return.
This already affects __syscall_poll and would also affect the new
__syscall_ppoll.