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
Process.fork has dangerous semantics #6421
Comments
Yes, I'd like that too. We have |
Another reason: the experiments on trying to bring multithread support to Crystal were difficult to achieve, and getting |
I used your example and ran it... got a similar input... I have scanned over the source code to see what's going on. I tweaked your example and ran a comparable in Ruby for expected output. I am fearful of runaway processes, so I will always use a wait. ` t = Process.fork do chan.receive I didn't quite the same output:
These are going to be fun bugs to eradicate... :P @asterite I think that the lack of windows operability shouldn't mean that we need to discard the whole thing. Now that Microsoft is going into the deepend on interoperability with Linux... these functions will be able to be accessed by MS users. There is always spawn or other ways to do it also for windows users. |
Ah, the output was from I think the only way to improve fork while keeping it would be implement forkone semantics but then we'll have to worry about mutexes and deadlocks. I am not sure if that will be considered an improvement. see |
I think fork should be kept if possible, maybe as suggested by enforcing fork and exec pattern. |
Random idea: can we allow it only before any user fibers, or generally any fibers not explicitly marked as safe for fork, are created? |
|
fork is need for example here: https://github.com/kostya/curl-downloader, i not find any efficient ways except fork (i tried scheduler mapper to events, threads, curl multi). just write some danger comment for all who wants to use fork. |
@forksaber you wrote above "PS: I've been implementing a supervisor clone in crystal which gave me an opportunity to look deeper into Crystal's internals." Could you explain how you did this? @forksaber on the second note, I got similar type erratic behavior also, but I am impatient so I wasn't going to wait 14 min for the sleep LOL. I tried it at different times, but curiously when I rewrote with the wait, it sort of straightened up the behavior. I guess that it seems that most folks want to keep the fork, perhaps remove the exec, and put in strong warning, use at your own risk, explaining some possible side effects. |
I agree with the "keep fork as-is (but fix as many bugs as we can) but mark it as super dangerous" idea. We should provide a "sanitized" safe fork that works before fibers have been spawned too. |
@98-f355-f1 You can check out my supervisor implementation here https://github.com/forksaber/supervisor |
I think we should remove plain fork is not supported on win32 at all. That makes it a very platform-specific feature which we would like to avoid in stdlib. This answer on SO explains the issues with fork in Golang, which should apply exactly to Crystal as well: https://stackoverflow.com/a/28371586/7401689 As a replacement, fork could be made available in a platform-specific shard which can be added as a dependency for the few use cases where you would really want to use bare fork behaviour. fork immediately followed by exec would obviously still be required as implementation for |
|
I think we can keep platform depended apis along with their logical modules but mark them as so. If someone uses them he should know about consequences (i.e non-portability). See my proposal about this concept - #8524. |
I finally changed my mind on
Let's not bother. |
The only concevable use of fork would be to insert safe privilege-dropping commands like a chroot or setrlimit between the fork and the exec. This would be useful for a container scheduler for example. I would support an unsafe non-documented API to make these happen if someone was interested in the capability, but otherwise i'm fully in support of pulling fork. |
That would certainly be better placed in a documented API of a dedicated shard. |
@straight-shoota well |
forkall
(Current)Process.fork
currently hasforkall
semantics with respect to fibers. What this means is that all the fibers which were alive during theProcess.fork
call remain alive in the forked process.Advantages
Disadvantages
Since fd/sockets are shared across forks, fibers which were doing writes will do double writes which will cause:
Similarly, fd/sockets reads may get divided across forks. So, both processes will see
An example of double writes:
Expected output:
Actual output:
IMO, these are really severe disadvantages so these semantics ought to change.
Alternatives
1.
fork
like semantics (linux/unix)In this scenario, only the fiber invoking
Process.fork
lives on in the forked process. Rest of the fibers are either collected(majority of them) and a few core runtime fibers likeSignalHandler loop
are recreated.Advantages
Disadvantages
pthread_atfork
like api for fibers may help here though i am not sure how successful that will be.2. Disable fork without exec (go/erlang)
This approach is in line with what go and erlang takes. They find
fork
too difficult to implement correctly in a multi-threaded/multi-fiber/multi-actor environment that they don't even bother implementing it.Disadvantages
exec()
s to a crystal binary.Conclusion
Personally, I would prefer that fork without exec be removed from the stdlib so there's no risk of deadlocks or data corruption. If that's unacceptable, even implementing
fork
semantics is still a step in the right direction.PS: I've been implementing a
supervisor
clone in crystal which gave me an opportunity to look deeper into Crystal's internals.#6416 is also caused by
forkall
behaviour. I'll provide a root cause analysis in that thread.The text was updated successfully, but these errors were encountered: