-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Use shutdown hook to delete temp files instead of File.deleteOnExit #4578
Use shutdown hook to delete temp files instead of File.deleteOnExit #4578
Conversation
Previously, the JRubyClassLoader would make a .deleteOnExit() call for the temp directory and each temp file it needed to create. This results in the string for each registered path being stored in a hash that increases each time a new URL is loaded or new ScriptingContainer is created. This commit removes the .deleteOnExit() calls and instead registers a shutdown hook just after the temp directory is initially created. At shutdown time, the hook would delete any remaining files and the temp directory itself. This should effectively result in the same user behavior as before but with the benefit of the temp path strings no longer taking up an increasing amount of memory the longer the process runs.
This issue affects our users who need to have the ability to have |
looks good - shutdown-hooks do not need try-catch right (on |
I'm thinking we should do both, no? I want to ensure that abnormal termination of JRuby still has the JVM deleteOnExit to fall back on. |
it seems to be common approach to delete directory and its content in custom shutdown hook and not using deleteOnExit |
@kares, looking at the Javadocs for File.delete, it appeared that the only exception which might be thrown is a For the case that the process is unable to delete an entry (e.g., for a non-empty directory), I've seen that |
@headius, thanks for taking a look at this PR. My intent with this PR was to replace the use of the JVM
Over time, then, we end up with a new set of strings being added to the hash each time a new container is created - even though the corresponding tempDir jars should no longer be in use after the corresponding With the custom shutdown hook approach on this PR, I believe we still get the same basic behavior as what the With the changes in the current PR, I've done some manual testing locally with different kinds of process termination - including hitting CTRL^C to kill a foreground process, sending a SIGTERM to kill a background process, and throwing an exception post-container creation in a way which kills the process. In each of the cases, I saw all of the tempDir jar files and the tempDir itself be cleaned up in conjunction with the process dying. Are there other cases you can think of where the |
thanks - have read the java-docs seems that exception handling is unnecessary. although I believe there might have been some implementation issues on IBM JREs in the past (maybe I am wrong). still, the current approach is a bit different -
you could test-out a (unlikely) case where the directory is not deletable - some files fails to get rm-d. its probably easy to simulate if you just create a new file manually in the dir while its running. although its not that important - the behaviour in this case should be similar to what would happen with |
Thanks, @kares.
I'm happy to do that if you'd like. Would doing something simple like what the for (File f : tempDir.listFiles()) {
try {
f.delete();
} catch (Exception ex) {
LOG.debug(ex);
}
}
try {
tempDir.delete();
} catch (Exception ex) {
LOG.debug(ex);
} What do you think?
That makes sense. I have tried doing that. For example, I tried putting a file under a subdirectory within the temp jruby directory. Even though the new code would not be able to delete the subdirectory at shutdown in that case, I don't see any errors written to stdout or stderr - even without the extra try/catch logic. The Anything else you can think of that I should look into in order to decide whether or not this could be merged? Thanks again, all! |
Previously, a warning would be generated each time the JRuby InternalScriptingContainer would fail to remove a file during its termination. Per work in the upstream JRuby project that we hope will land soon, jruby/jruby#4578, a JRuby shutdown hook could remove some of these files before the InternalScriptingContainer has a chance to remove them. This commit removes the warnings to avoid creating unnecessary noise in the logs related to the timing of file deletions.
Is there any more information needed to decide whether this is a change that the project would be willing to accept? Thanks again your time reviewing. |
think its good enough - what do you think of increasing the log level but without the stack-trace : for (File f : tempDir.listFiles()) {
try {
f.delete();
} catch (Exception ex) {
LOG.debug(ex);
}
}
try {
tempDir.delete();
} catch (Exception ex) {
LOG.info("failed to delete temp dir " + tempDir + " : " + ex);
} you know there's something wrong - one is likely to get more details (stack-traces) when running with debug mode ... or is that too noisy? |
For any cases where the shutdown hook in the JRubyClassLoader encounters an exception when trying to cleanup temp files, the exception should now be caught and logged.
Previously, the JRubyClassLoader would make a .deleteOnExit() call for
the temp directory and each temp file it needed to create. This results
in the string for each registered path being stored in a hash that
increases each time a new URL is loaded or new ScriptingContainer is
created.
This commit removes the .deleteOnExit() calls and instead registers a
shutdown hook just after the temp directory is initially created. At
shutdown time, the hook would delete any remaining files and the temp
directory itself. This should effectively result in the same user
behavior as before but with the benefit of the temp path strings no
longer taking up an increasing amount of memory the longer the process
runs.