style95 commented on a change in pull request #4430: Update docker client
version to 18.06.3
URL:
https://github.com/apache/incubator-openwhisk/pull/4430#discussion_r280306958
##########
File path:
core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InvokerReactive.scala
##########
@@ -96,7 +97,12 @@ class InvokerReactive(
"--ulimit" -> Set("nofile=1024:1024"),
"--pids-limit" -> Set("1024")) ++ logsProvider.containerParameters)
containerFactory.init()
- sys.addShutdownHook(containerFactory.cleanup())
+
+
CoordinatedShutdown(actorSystem).addTask(CoordinatedShutdown.PhaseBeforeActorSystemTerminate,
"cleanup runtime containers") {
Review comment:
This also newly comes with the recent version of Docker.
I am not quite sure the reason yet, but this `cleanup` procedure is
terminated by `SIGTERM`.
(As you know, `docker stop` sends `SIGTERM` to the PID1 in the container,
wait for 10s by default and send `SIGKILL` if the process does not stop)
In the previous version, it seems this procedure was not terminated by
`SIGTERM`, but now it is.
So I changed this to `CoordinatedShutdown`.
Since the previous shutdown hook does not guarantee the task run, I think it
would provide a better option for us.
>def addShutdownHook(body: ⇒ Unit): ShutdownHookThread
...
>
>Note that shutdown hooks are NOT guaranteed to be run.
(Reference:
https://www.scala-lang.org/api/current/scala/sys/index.html#addShutdownHook(body:=%3EUnit):scala.sys.ShutdownHookThread)
One thing to note is, it does not respect the result from `cleanup()` method
as it returns nothing(`Unit`).
But I think it provides the same level of guarantee with the previous
version, so it would be fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
