-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
pipe() doesn't create a blocking pipe #13214
Comments
Blocking is very tricky on the web platform. Unless you are running on the main thread you can't really block. You have to condtant yield to the event loop. In emscripten we can do this using ASYNCIFY but it has a cost and is not on by default. If you can think of a reasonable way simulate/emulate a blocking pipe we would welcome it. Or a better way to report that we don't support such things? |
Sorry I mean to write "Unless you are running off the main thread". |
I also tested the same snippet in a background thread, and the result was the same - it returns EAGAIN instead of blocking. Also if a primitive like that isn't supported on a main thread, then probably the following docs need to be updated (they're written to suggest that all such blocking operation work on the main thread, even at a cost of runtime penalty): https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread |
If you can think of a way to discuss that issue in that document that could be good yes. That document mostly talks about threading APIs though. Perhaps https://emscripten.org/docs/porting/asyncify.html could be another possible place to mention this since its one way it could be possible to create a blocking pipe. Since emscripten is single process and doesn't support any kind of fork() i'm not sure how useful pipe really is for our users. Maybe for multi-threaded application one might want to pipe data between threads? I'm curious what you use case for them is? |
It's true that And, yeah, it's confusing to see that the |
I agree that maybe failing to create the pipe at all might be more clear. I'd be happy with that approach if the user is building in a configuration where blocking pipes are clearly not possible (e.g. without ASYNCIFY (which could one day allow for actual blocking pipes although I don't think it does today)). Alternatively we could change the error return into a fatal abort and report "attempt to read from empty pipe without NONBLOCK.. this is not supported on emscripten because we can't block". Then certain programs that don't need blocking would continue to work and developers who rely on blocking would see a fatal error. Either way I think is fine given that there are probably very few uses for pipe in emscripten today. |
Thanks, these options will be a good way to tell developer about the problem. Also would be great to mention it in docs - it helps when you know which unsupported functions you have to look for in the codebase. |
I had exact same question here: and answer then was that its not possible. But now that we have support for atomic.waitAsync in browser: couldn't this be done using native browser feature (instead of asyncify which could also use same browser feature)? Is support for atomics coming in emscripten/webassembly also? |
Sure if you want to do blocking pipes with multiple threads it could be possible to do with atomics. We do a lot of inter-thread communication within emscripten using atomics already. What won't work (at least not without ASYNICIFY) is a single threaded app where the pipe is fed, for example, from some kind of JS event (like a fetch request). |
This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant. |
The immediate reason for this is that pipes are broken in the Emscripten runtime, see emscripten-core/emscripten#13214. But if we can drop the use of a pipe for other platforms, too, why not. Without this, when attemting to run Collabora Online as WASM, I get: Aborted(Assertion failed: nRet == 1, at: .../vcl/headless/svpinst.cxx,538,DoYield) It is quite possible that the code could be simplified drastically. I only replaced the use of a pipe with hopefully equivalent use of a queue, a condition variable, and a mutex. Change-Id: I9259ba36afeabce6474a1aec827d01bcbbd4412b Reviewed-on: https://gerrit.libreoffice.org/c/core/+/144944 Reviewed-by: Michael Meeks <michael.meeks@collabora.com> Tested-by: Jenkins
Stale bot intends to close this issue, but I believe it'll still be a valuable addition: POSIX doesn't forbid in-process usages of |
I guess we could error on pipe creation if |
I'll probably not have cycles to work on this anymore soon. Erroring and/or logging would definitely be much more developer-friendly! Implementing POSIX behavior would be even better, as this would mean developers don't have to rewrite |
Any kind of blocking behaviour on the web is on the more complex/hard problems we face. So failing on the creation of blocking pipes seems like that most reasonable solution for now. |
The immediate reason for this is that pipes are broken in the Emscripten runtime, see emscripten-core/emscripten#13214. But if we can drop the use of a pipe for other platforms, too, why not. Without this, when attemting to run Collabora Online as WASM, I get: Aborted(Assertion failed: nRet == 1, at: .../vcl/headless/svpinst.cxx,538,DoYield) It is quite possible that the code could be simplified drastically. I only replaced the use of a pipe with hopefully equivalent use of a queue, a condition variable, and a mutex. Change-Id: I9259ba36afeabce6474a1aec827d01bcbbd4412b Reviewed-on: https://gerrit.libreoffice.org/c/core/+/144944 Reviewed-by: Michael Meeks <michael.meeks@collabora.com> Tested-by: Jenkins
Shims read(2) functionallity by providing an alternative read() function called wasm_read() that gracefully handles blocking pipes. This is neecessary because Emscripten does not support blocking pipes and instead returns EWOULDBLOCK. See: * #951 * emscripten-core/emscripten#13214 ## Testing instructions Confirm the `proc_open()` tests pass on CI. This PR adjusts a few of them to make sure the output is read without the tricky sleep() call.
Shims read(2) functionallity by providing an alternative read() function called wasm_read() that gracefully handles blocking pipes. This is neecessary because Emscripten returns EWOULDBLOCK when reading from a blocking pipe, whereas a clang-compiled program would wait until data becomes available. See: * #951 * emscripten-core/emscripten#13214 ## Other changes This PR also ships: * A regexp fix for `@php-wasm/cli` to preserve a trailing whitespace when rewrite PHP-related spawn shell commands. * A fix to correctly propagate the child process exit code back to PHP. The WordPress bootstrap script for PHPUnit needs that to work correctly. ## Motivation ### wp-cli Without this PR, running `wp-cli.phar` doesn't output anything. The output is piped through a pager like `less` using `proc_open` and then displayed by reading from a blocking pipe. With this PR, running `wp-cli.phar` returns its regular help message as the pager pipe can be read: ``` NAME wp DESCRIPTION Manage WordPress through the command-line. SYNOPSIS wp <command> SUBCOMMANDS cache Adds, removes, fetches, and flushes the WP Object Cache object. cap Adds, removes, and lists capabilities of a user role. cli Reviews current WP-CLI info, checks for updates, or views defined aliases. ... ``` ### PHPUnit With this PR, Playground can run PHPunit on `wordpress-develop`! ``` TMPDIR=/tmp PHP=8.2 node --loader ../plugins/playground/packages/nx-extensions/src/executors/built-script/loader.mjs ../plugins/playground/dist/packages/php-wasm/cli/main.js ./vendor/bin/phpunit --no-coverage -v (node:87723) ExperimentalWarning: Custom ESM Loaders is an experimental feature. This feature could change at any time (Use `node --trace-warnings ...` to show where the warning was created) Installing... aaabb int(0) Running as single site... To run multisite, use -c tests/phpunit/multisite.xml Not running ajax tests. To execute these, use --group ajax. Not running ms-files tests. To execute these, use --group ms-files. Not running external-http tests. To execute these, use --group external-http. PHPUnit 9.6.15 by Sebastian Bergmann and contributors. Runtime: PHP 8.2.10-dev Configuration: /Users/cloudnik/www/Automattic/core/wordpress-develop/phpunit.xml.dist Warning: Your XML configuration validates against a deprecated schema. Suggestion: Migrate your XML configuration using "--migrate-configuration"! ........................................................... 59 / 17622 ( 0%) ........................................................... 118 / 17622 ( 0%) ............................FFFFFF......................... 177 / 17622 ( 1%) ........................................................... 236 / 17622 ( 1%) ``` ## Testing instructions Confirm the `proc_open()` tests pass on CI. This PR adjusts a few of them to make sure the output is read without the tricky sleep() call. Related: * #827 * #930 CC @mho22
On Emscripten, when reading from a pipe created via
pipe()
, theread()
call returns immediately withEAGAIN
when there's no data in the pipe. This contradicts the standard behavior specified by POSIX:https://man7.org/linux/man-pages/man2/read.2.html
https://man7.org/linux/man-pages/man7/pipe.7.html
Given that
pipe()
doesn't set theO_NONBLOCK
flag, I believe the Emscripten's behavior with returningEAGAIN
is wrong.Sample program:
This program, when run under Emscripten, fails with "Resource temporarily unavailable". When the same program is run on Linux using standard g++ and libc, it correctly blocks in
read()
.P.S. The program also demonstrates that, in case
write()
was previously called, the data is successfully read from it under Emscripten.The text was updated successfully, but these errors were encountered: