search

com-lihaoyi / os-lib

OS-Lib is a simple, flexible, high-performance Scala interface to common OS filesystem and subprocess APIs
Other
672 stars 63 forks source link

Added proper SIGTERM/SIGKILL handling for sub-processes #286

Closed j-mie6 closed 1 month ago

j-mie6 commented 1 month ago

Fixes #284.

The idea is to introduce a new flag to both os.proc.call and os.ProcGroup.call, as well as backing implementations in os.ProcessLike. The flag, timeoutGracePeriod, allows for a configurable use of SIGKILL, which forcibly kills a process, after the issue of a SIGTERM, which allows a process to clean up. The values for the flag have the following behaviours:

value behaviour
-1 When timeout != -1, only a SIGTERM will be issued, requiring the child process to gracefully terminate.
0 When timeout != -1, only a SIGKILL is issued, demanding the child terminate immediately with no chance of graceful cleanup.
n > 0 timeout != -1, first issue a SIGTERM and then wait for a further n milliseconds before issuing a SIGKILL, this provides a reasonable timeframe for process cleanup, but accounts for misbehaving processes.

For now, the default has been set to timeoutGracePeriod = 1000.

j-mie6 commented 1 month ago

@lihaoyi other than the documentation for ProcessLike, and any additional tests, this seems pretty much good to go. I have a couple of questions above that it would be nice to discuss. Can I get a CI run please?

lihaoyi commented 1 month ago

I think this looks good.

1000 seems a bit long for a grace period; what do you think about something shorter, like 50ms or 100ms? I don't want to change the behavior too much from the status quo, which terminates the process immediately

j-mie6 commented 1 month ago

I think this looks good.

1000 seems a bit long for a grace period; what do you think about something shorter, like 50ms or 100ms? I don't want to change the behavior too much from the status quo, which terminates the process immediately

Sure, 100ms is fine. Do note that it's unlikely to wait the full 1000ms anyway, because SIGTERM exits fast unless there is cleanup to do or the process ignores it anyway.

lihaoyi commented 1 month ago

I think this looks good. @j-mie6 if you're happy with too it I'll merge it and cut a release

j-mie6 commented 1 month ago

I've patched up the documentation now, so I'm happy to ship it if you are :)