ocean icon indicating copy to clipboard operation
ocean copied to clipboard

SafeFork doesn't reap the child processes

Open nemanja-boric-sociomantic opened this issue 7 years ago • 9 comments

Ocean provides wrapper around fork: https://github.com/sociomantic-tsunami/ocean/blob/v3.x.x/src/ocean/sys/SafeFork.d#L120 which calls fork, but it doesn't do anything with the SIGCHLD events, resulting in leaving zombie processes on the system.

SafeFork should at least document that the user should either explicitly ignore SIGCHLD signal, specify SA_NOCLDWAIT flag, or call wait when signaled to release the system resources used by the process.

One other tricky part is the interaction with the EpollProcess's SIGCHLD handler. Maybe GlobalProcessMonitor should be decoupled from EpollProcess and used for both EpollProcess and SafeFork.

Maybe we should instead just deprecate SafeFork and use threads? :P

(I am not sure what exactly to implement for v4.0.0 to resolve this)

Our team use SafeFork heavily

Curious. Do you mind few examples? I thought it is something @nemanja-boric-sociomantic needed in DLS as it doesn't look like utility you would want for a general purpose.

I am specifically curious why one would want to use fork for this purpose and not a thread unregistered from GC.

We use it to asynchronously dump aggregations which can be a few Mb to a few Gb.

I am specifically curious why one would want to use fork for this purpose and not a thread unregistered from GC.

Same reason that CDGC uses it: COW memory (cheap snapshot + pay for what you use approach).

I think there's a way to make this a non-breaking change. The main problem with this is that processes are using SIGCHLD to notify the parent they exited, but the signal handler is already taken by ocean's very own EpollProcess. Moving signal handling one level up, so that both SafeFork and GlobalProcessMonitor can use it would work. I propose we move this to the v4.1.0.

Burgos avatar Dec 22 '17 23:12 Burgos

Curious. Do you mind few examples? I thought it is something @nemanja-boric-sociomantic needed in DLS as it doesn't look like utility you would want for a general purpose.

Yeah, it's not something DLS is using, but I've seen the examples where there were dozens of zombies wondering around the system in the SafeFork's application. I agree it's not something one would want for a general purpose, but there's many tricky bits around fork that can go terribly wrong and that should be kept away from the user, so that makes me think this is a good thing to have in Ocean.

I am specifically curious why one would want to use fork for this purpose and not a thread unregistered from GC.

In addition to @mathias-lang-sociomantic 's answer, here' semi-relevant issue: https://github.com/sociomantic-tsunami/dhtproto/issues/82#issuecomment-353023100

Burgos avatar Dec 22 '17 23:12 Burgos

I propose we move this to the v4.1.0

I gladly agree :) Worst case we can introduce new module with a fixed implementation.