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
Unix.{system,open_process_{in,out,full}} are not threadsafe #5266
Comments
Comment author: till Attached a file that implements a thread safe fork-exec. The signature of the function is: external fork_exec : I'd be happy to clean up this file and make it a patch to the ocaml Unix library if you want. |
Comment author: gmelquiond I have been experimenting with your code since it has some nice properties that the original implementation of create_process lacks. In particular, it has proper error handling (e.g. calling _exit in case of error) and sends back to the parent process the errno value. It also avoids a rare race condition that can bring the system to its knees, since it does not allow the parent process to perform a major collection before the child process has finished discarding the obsolete address space (at least for single-threaded programs). And finally, it supports setting the working directory, as does the system call on Windows. Thanks a lot. I have encountered a few issues though, that I will list here in case someone else stumbles upon your code:
|
Comment author: till Good points. This code was surprisingly hard to write and I am sad to see that I still let mistakes slip through (especially the fd leak). I will try to submit a fix soon. Coming back to the pfd issue: can you think of a way that would make the parent deadlock? pdf[0] being set cloexec should be closed when the forked-side fails (and thus exits) or succeed (and exec's) |
Comment author: gmelquiond It has been some time, so my memories are a bit hazy. The read on pfd[0] will abort only if all the write ends of the pipe are closed, which is not the case if pfd[1] is still open in the parent, hence the deadlock. Am I missing something? Anyway, I have uploaded the variant of your code I have been using successfully. |
Comment author: till Oh. Palm/forehead! that is definitely a bug. I must have uploaded that file before fixing it. There's no way this could work. Thanks! |
Comment author: @mshinwell FWIW, https://github.com/janestreet/shexp contains a process creation library that shouldn't suffer from async signal safety problems. (I think there may be an update to that library soon.) |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
Multicore OCaml will probably need to restrict uses of |
Those Unix process-creation functions where reimplemented by @xavierleroy using |
Original bug ID: 5266
Reporter: till
Status: acknowledged (set by @damiendoligez on 2011-05-17T12:16:32Z)
Resolution: open
Priority: normal
Severity: feature
Target version: later
Category: otherlibs
Tags: patch
Monitored by: mehdi
Bug description
When calling fork in a multi-threaded program the forked side can only call async-signal-safe functions [http://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html] this means that we probably cannot rely at all on using the ocaml runtime on the forked side.
Even if we can use the ocaml runtime (because we do no allocation and therefor are compiled down to fairly straghtforward c calls) we shouldn't call the c [exit] function on errors but rather the [_exit] function which doesn't call the [at_exit] registered handler (similarly we do not want to run the ones registered in ocaml). These are not necessarily async-signal-safe and might perform flushing/cleaning up that only makes sense in the parent process (e.g.: cleaning up temporary files).
File attachments
The text was updated successfully, but these errors were encountered: