-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix select didn't work with pipes and the timeout argument
#25523
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
Conversation
89b85b8 to
05bd5fd
Compare
|
Thanks for the PR @ktock. This looks like a fairly large and complex PR, and I'm out sick right now I'm afraid so I'm not sure when I will have time to go through it. CC'ing @atrosinenko who wrote the original pipefs back in #4935. Also @kripken and @tlively who might be interesting the multithreaded event stuff. |
5fce398 to
3938766
Compare
|
Rebased. Could we move this forward? |
|
I am unfortunately not familiar with this code (or with native pipes either). It looks like the PIPEFS code began in #4378 / #4935 by @cynecx and @atrosinenko - would one of you be interested to review this perhaps? |
d7f75dc to
16b7865
Compare
I've just simplified the patch by removing changes in libpthread.js. Instead, this patch uses emscripten_proxy_sync_with_ctx to synchronize the thread worker and the main worker. This patch allows pipefs and setTimeout to unblock the worker by calling emscripten_proxy_finish when an event occurs. I've added some wrapper functions for the proxying-related APIs so that the JS implementation of newselect can use them. |
5cef212 to
ac6c045
Compare
aaa6575 to
6079e85
Compare
|
The failure in
|
|
Circle CI should be fixed in #25955 |
After emscripten-core#25523 this function is no longer proxied itself and therefore doesn't need to live in JS at all.
After emscripten-core#25523 this function is no longer proxied itself and therefore doesn't need to live in JS at all.
After emscripten-core#25523 this function is no longer proxied itself and therefore doesn't need to live in JS at all.
After emscripten-core#25523 this function is no longer proxied itself and therefore doesn't need to live in JS at all.
After emscripten-core#25523 this function is no longer proxied itself and therefore doesn't need to live in JS at all.
After emscripten-core#25523 this function is no longer proxied itself and therefore doesn't need to live in JS at all.
After #25523 this function is no longer proxied itself and therefore doesn't need to live in JS at all.
These were added unnecessarily in emscripten-core#25523. `_emscripten_proxy_newselect_finish` was added to `create_pointer_conversion_wrappers` in emscripten.py which makes these conversions automatic.
These were added unnecessarily in #25523. `_emscripten_proxy_newselect_finish` was added to `create_pointer_conversion_wrappers` in emscripten.py which makes these conversions automatic.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
Sorry for the slow reply, and thank you for the follow-up PRs. I've also seen the ongoing PR #25990 and added comments there as well. Please let me know if there is something I can do for these features. |
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see #25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This change improves the implementation of poll() to support blocking when called from threads (see emscripten-core#25523) and moves the select-based-on-poll implemenation from being wasmfs specific to be always being used.
This PR fixes an issue that the timeout argument for the select syscall wasn't applied to pipes and it returned immedeately instead of waiting for events.
Although the select syscall passes the timeout value to the
pollmethod of the stream implementations, this can't handle multiple fds because a fd can't notify readiness while another is blocking inpoll. As a result, using the select syscall with a combination of pipes and other streams (e.g. PTY) can be problematic.To address this, this PR implements a callback-based event notification. Each stream implementation's
pollinvocation receives a callback, allowing it asynchronously notify the select syscall when it becomes ready. The select syscall blocks until one of these callbacks is triggered or the timeout expires. This behviour is enabled only when PROXY_TO_PTHREAD is enabled to avoid blocking the main worker.To maintain compatibility with non-pipefs streams, the select syscall allows stream implementations to ignore the callback and synchronously return the event status instead. In such cases, the select syscall still updates the flags accordingly.