-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add flag to register shutdown hooks for os.call
and os.spawn
APIs, overhaul destroy
APIs
#324
Conversation
os.call
and os.spawn
APIsos.call
and os.spawn
APIs, overhaul destroy
APIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename shutdownHook
to something like exitWithJvm
or stopWithJvm
. We don't really support any custom shtudown hook. We just use the shutdown hook mecahnism internally to stop the process when the JVM is stopped.
Ended up going with |
@@ -1725,7 +1725,9 @@ os.call(cmd: os.Shellable, | |||
mergeErrIntoOut: Boolean = false, | |||
timeout: Long = Long.MaxValue, | |||
check: Boolean = true, | |||
propagateEnv: Boolean = true): os.CommandResult | |||
propagateEnv: Boolean = true, | |||
shutdownGracePeriod: Long = 100, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the time unit used for shutdownGracePeriod
? Should we name it shutdownGracePeriodMsec
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this only provided for bin-compat? Shouldn't we deprecate it therefore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this one is only for bin-compat, so we should deprecate it for removal.
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
.call
and.spawn
invocationsConsolidate
SubProcess#destroy
andSubProcess#destroyForcibly
into a singleSubProcess#destroy
method which takes some default parameters, allowing the user to chooseasync = true
or configure theshutdownGracePeriod: Long
.The default behavior of
SubProcess#destroy
has 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