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?
| (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?
thiblahute
force-pushed
the
ppoll_proxy
branch
2 times, most recently
from
2910aa5 to
0b3ab8f
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.
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.