Add flag to register shutdown hooks for os.call and os.spawn APIs, overhaul destroy APIs #324
Conversation
lihaoyi changed the title os.call and os.spawn APIsos.call and os.spawn APIs, overhaul destroy APIs 
| Ended up going with |
| check: Boolean = true, | ||
| propagateEnv: Boolean = true): os.CommandResult | ||
| propagateEnv: Boolean = true, | ||
| shutdownGracePeriod: Long = 100, |
There was a problem hiding this comment.
What's the time unit used for shutdownGracePeriod? Should we name it shutdownGracePeriodMsec?
There was a problem hiding this comment.
I'll probably leave it as is, the scaladoc says it's in milliseconds so that should be enough
| destroyOnExit = destroyOnExit | ||
| ) | ||
| } | ||
| def apply( |
There was a problem hiding this comment.
Is this only provided for bin-compat? Shouldn't we deprecate it therefore?
There was a problem hiding this comment.
I've had issues with @deprecated things being accidentally resolved here before, but will add a comment like the other forwarders
| destroyOnExit = destroyOnExit | ||
| ) | ||
| } | ||
| def apply( |
There was a problem hiding this comment.
Looks like this one is only for bin-compat, so we should deprecate it for removal.
This might have been the cause of a lot of flakiness that seems to have gone away with #4254, as the server exiting caused the `runBackground` calls to exit causing the http servers to exit and fail to pick up requests. Might have been caused by com-lihaoyi/os-lib#324 which made `destroyOnExit` the default for spawned subprocesses. This PR explicitly disables `destroyOnExit` for the subprocesses where `background = true` Covered by a new `integration.invalidation` test that runs under both `server` and `fork`, that previously failed when run under `fork`
2 participants
Backports the functionality from Mill to allow us to register shutdown hooks such that when the parent process terminates the subprocesses are shut down as well. This allows the same logic to be used consistently across all
.calland.spawninvocationsConsolidate
SubProcess#destroyandSubProcess#destroyForciblyinto a singleSubProcess#destroymethod which takes some default parameters, allowing the user to chooseasync = trueor configure theshutdownGracePeriod: Long.The default behavior of
SubProcess#destroyhas changed to block on the subprocess actually exiting, which I think is more intuitive. The old behavior is available underdestroy(async = true)Best reviewed with
Hide whitespace. Added some simple unit tests to assert the new functionality, existing unit tests should assert on existing workflows