Fix: the shutdown hook should be removed when the process ends to avoid a memory leak. #173
Conversation
…id a memory leak.
| (.addShutdownHook hook)) | ||
| (if-before-jdk8 | ||
| nil ;; throwing an exception here would commit breakage, therefore accept the memory leak for jdk8 | ||
| (-> (.onExit proc) |
There was a problem hiding this comment.
What if there is also a :exit-fn?
There was a problem hiding this comment.
Ah, I tested this, it works because you can call .onExit + .thenRun multiple times.
user=> (require '[babashka.process :as p])
nil
user=> (let [p (:proc (p/process "sleep" "1"))] (-> (.onExit p) (.thenRun (fn [] (prn :foo)))) (-> (.onExit p) (.thenRun (fn [] (prn :bar)))))
#object[java.util.concurrent.CompletableFuture 0x77049094 "java.util.concurrent.CompletableFuture@77049094[Not completed]"]
user=> :bar
:foo
There was a problem hiding this comment.
@maxweber Can I ask you what kind of stuff you were doing in the shutdown hook which caused the memory build-up?
There was a problem hiding this comment.
I think the only downside to this PR is that the shutdown hook won't be executed if the process finishes earlier than the exit of the parent process, but I guess this is fine since the shutdown hook is mostly there for killing the process and its children.
There was a problem hiding this comment.
Hmm, one corner case: if you previously had :shutdown p/destroy-tree then the PR behavior might result in not having the children killed if the process exits before the parent process?
There was a problem hiding this comment.
@maxweber Yes, I think that would be reasonably, although I don't know exactly what would happen if you call p/destroy-tree in :exit-fn but we could try to find out :)
What I was wondering: did you use any custom :shutdown hook or just p/destroy(-tree)?
There was a problem hiding this comment.
That seems to just work. So adding (shutdown res) after removing the shutdown hook would work I think to ensure it's still called.
There was a problem hiding this comment.
@borkdude I'm using p/destroy(-tree) indirectly via the defaults of process/shell. In our code I just set :shutdown nil to fix the memory leak for the moment :)
Adding (shutdown res) after the hook removal sounds good 👍 One thought, maybe starting the hook Thread instead would be safer? If someone does something 'fancy' in the hook that blocks very long or just has an error that causes an exception usually only during the shutdown of a long running app?
There was a problem hiding this comment.
Agreed. Also please update the docstring to document this new behavior.
There was a problem hiding this comment.
@borkdude as discussed the :shutdown hook is now executed as soon as the child process ends and the docstring has been extended.
|
@maxweber Do you use this library from the JVM and do you need a new release? |
|
Already released a new version 0.6.23 |
|
Awesome 🥳 Thanks a lot. |
2 participants
Thanks a lot for creating babashka/process , it is awesome and helps us a lot 🥳
We found this edge case due to an OutOfMemory issue on one of our production servers. It only occurs after several days. We call process/shell three times per second. Below a suggestion for a potential fix.