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

Merged
merged 2 commits into from May 24, 2017

Conversation

Projects
None yet
4 participants
@camlow325
Contributor

camlow325 commented Apr 28, 2017

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.

Use shutdown hook to delete temp files instead of File.deleteOnExit
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.
@camlow325

This comment has been minimized.

Contributor

camlow325 commented Apr 28, 2017

This issue affects our users who need to have the ability to have ScriptingContainer instances be recycled over time -- without restarting the Java process for performance. We do this in order to protect our users from having custom code extensions that they may have written (which may have some memory leaks underneath) cause the process to crash frequently with OOM errors. Note that this issue is also related to, but does not solve, #3928.

@kares

This comment has been minimized.

Member

kares commented Apr 29, 2017

looks good - shutdown-hooks do not need try-catch right (on tempDir.delete())?
in case someone else writes at the location (e.g. creating a dir) it would fail while previously it did not ...

@headius

This comment has been minimized.

Member

headius commented Apr 29, 2017

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.

@mkristian

This comment has been minimized.

Member

mkristian commented Apr 30, 2017

it seems to be common approach to delete directory and its content in custom shutdown hook and not using deleteOnExit

@camlow325

This comment has been minimized.

Contributor

camlow325 commented May 1, 2017

shutdown-hooks do not need try-catch right (on tempDir.delete())?

@kares, looking at the Javadocs for File.delete, it appeared that the only exception which might be thrown is a SecurityException. I had noticed that in the JDK's implementation of its hook behind the deleteOnExit method was also using File.delete but wasn't trying to catch SecurityException or any other exception around the call.

For the case that the process is unable to delete an entry (e.g., for a non-empty directory), I've seen that File.delete() just returns false but doesn't throw an exception. If you think it would be safer to wrap the custom shutdown hook code in a try/catch that swallows any exceptions found, though, I'd be okay with making that change. What do you think?

@camlow325

This comment has been minimized.

Contributor

camlow325 commented May 1, 2017

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.

@headius, thanks for taking a look at this PR. My intent with this PR was to replace the use of the JVM deleteOnExit with a custom shutdown hook because, at least for our use case where JRuby ScriptingContainers can be recycled over time, the calls to deleteOnExit basically lead to a memory leak. For example, we have this sequence:

  1. ScriptingContainer created.
  2. 5 or more strings associated with tempDir jars are passed along to a deleteOnExit call.
  3. The JVM's DeleteOnExitHook stores each of the new strings in an internal hash.
  4. Some time later, the ScriptingContainer is terminated.
  5. Repeat steps 1 -4.

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 ScriptingContainer they had been created for has been terminated.

With the custom shutdown hook approach on this PR, I believe we still get the same basic behavior as what the deleteOnExit approach had provided but without the side effect of extra strings for the file paths continuing to stack up over time as containers are recycled.

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 deleteOnExit approach would cover cleaning up the temporary files / directory where the custom shutdown hook would not?

@kares

This comment has been minimized.

Member

kares commented May 2, 2017

For the case that the process is unable to delete an entry (e.g., for a non-empty directory), I've seen that File.delete() just returns false but doesn't throw an exception. If you think it would be safer to wrap the custom shutdown hook code in a try/catch that swallows any exceptions found, though, I'd be okay with making that change. What do you think?

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 - deleteOnExit having issues does not print anything, while an issue (uncaught exception) with the proposed changes will print a stack-trace to System.err. but that's fine with me, if its accepted we can move it forward to introduce some try-catch handling to log using JRuby's internal loggers.

Are there other cases you can think of where the deleteOnExit approach would cover cleaning up the temporary files / directory where the custom shutdown hook would not?

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 deleteOnExit.

@camlow325

This comment has been minimized.

Contributor

camlow325 commented May 9, 2017

Thanks, @kares.

if its accepted we can move it forward to introduce some try-catch handling to log using JRuby's internal loggers.

I'm happy to do that if you'd like. Would doing something simple like what the addURL method in the same class does make sense? Something like...

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?

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 deleteOnExit.

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 File.delete call for the directory just returns a silent false value without throwing any exceptions.

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!

camlow325 added a commit to camlow325/jruby-utils that referenced this pull request May 17, 2017

(MAINT) Remove warning for jar deletion failure
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.
@camlow325

This comment has been minimized.

Contributor

camlow325 commented May 17, 2017

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.

@kares

This comment has been minimized.

Member

kares commented May 18, 2017

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?

Wrap exceptions for JRubyClassLoader shutdown delete with log statements
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.
@camlow325

This comment has been minimized.

Contributor

camlow325 commented May 22, 2017

Thanks, @kares. I think your suggestion makes sense. I went ahead and added a commit onto the PR with that change, fc28208.

@kares kares added this to the JRuby 9.2.0.0 milestone May 24, 2017

@kares kares merged commit 205504b into jruby:master May 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment