Skip to content
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

Change fork() and system() to not return EAGAIN #7005

Closed
wants to merge 1 commit into from

Conversation

haukex
Copy link
Contributor

@haukex haukex commented Aug 16, 2018

The error code EAGAIN implies "try again later", but Emscripten's fork() will never succeed. Some C programs, when getting EAGAIN, will actually wait a few seconds and try again, sometimes many times. This patch changes fork() and system() to return ENOTSUP.

Any other error code is probably fine too, as long as it's not EAGAIN :-)

The error code EAGAIN implies "try again later", but Emscripten's fork() will never succeed. Some C programs, when getting EAGAIN, will actually wait a few seconds and try again, sometimes many times. This patch changes fork() and system() to return ENOTSUP.
@haukex
Copy link
Contributor Author

haukex commented Aug 17, 2018

I'll look at the test failures as soon as I can.

@kripken
Copy link
Member

kripken commented Aug 17, 2018

Thanks @haukex!

The change looks good. Test failures look like test outputs that need updating for the new error code. Should be easy, let me know if you run into a problem.

Also, please add yourself to AUTHORS.

@juj
Copy link
Collaborator

juj commented Aug 19, 2018

This change will cause the behavior to no longer conform to the Open POSIX Test Suite. The rationale for passing EAGAIN on these, as well as pthread_create():

https://github.com/kripken/emscripten/blob/595badf22b375229cad124a1cd3c9249373a6771/src/library_pthread.js#L538-L542

see also

97a091b#diff-be537b9f6dffd5af65894f62ef7416cfR27

comes from The Open Group's UNIX Specification at

http://pubs.opengroup.org/onlinepubs/7908799/xsh/fork.html
and http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_create.html

which has the wording

The pthread_create() function will fail if:
[EAGAIN]
The system lacked the necessary resources to create another thread, or the system-imposed limit on the total number of threads in a process PTHREAD_THREADS_MAX would be exceeded.

and

The fork() function will fail if:
[EAGAIN]
The system lacked the necessary resources to create another process, or the system-imposed limit on the total number of processes under execution system-wide or by a single user {CHILD_MAX} would be exceeded.

The OPEN Posix Test Suite enforces that no other error conditions are reported except those that are listed, which is why the intent is to output EAGAIN for "no more resources" to create threads.

I am open to diverging from this behavior, and returning ENOTSUP instead - could you do that for the pthread_join() entry point as well to be uniform?

See also occurrence of EAGAIN in tests, these look relevant:

/Users/clb/emsdk/emscripten/incoming/tests/pthread/test_pthread_malloc_free.cpp:
   45:       result = (rc != EAGAIN);

/Users/clb/emsdk/emscripten/incoming/tests/pthread/test_pthread_printf.cpp:
   27: 		assert(rc == EAGAIN);

/Users/clb/emsdk/emscripten/incoming/tests/pthread/test_pthread_supported.cpp:
   25: 	else assert(rc == EAGAIN);

/Users/clb/emsdk/emscripten/incoming/tests/pthread/test_pthread_thread_local_storage.cpp:
   54: 	else assert(rc == EAGAIN);

/Users/clb/emsdk/emscripten/incoming/tests/pthread/test_pthread_volatile.cpp:
   39:     int result = (rc != EAGAIN);

(change all of those to ENOTSUP)

@haukex
Copy link
Contributor Author

haukex commented Aug 22, 2018

Hi, sorry for the delayed response, it turns out my laptop isn't powerful enough to efficiently run the test suite, so I'll have to go to another machine. I'll update the branch with the changes (including pthread_join) as soon as I can, but it might be a few days until I can get around to it.

I agree that the change doesn't exactly conform to POSIX, but I figured that ENOMEM wasn't really a good option either.

By the way, thank you both for all your work, it's allowed me to port Perl to WebAssembly! https://webperl.zero-g.net/

@stale
Copy link

stale bot commented Sep 18, 2019

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 7 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Sep 18, 2019
@stale stale bot closed this Sep 25, 2019
@haukex
Copy link
Contributor Author

haukex commented Aug 1, 2023

Looks like this was fixed in #14173 (see also #14124).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants