Skip to content
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

PAYARA-4143 Replace usage of File.deleteOnExit() with manual deletion on exit #4201

Merged
merged 14 commits into from Sep 19, 2019

Conversation

@svendiedrichsen
Copy link
Contributor

svendiedrichsen commented Sep 5, 2019

Fixes #4199

I have replaced the usage of File.deleteOnExit with common-util FileUtils.deleteOnExit() to allow non-empty directories being deleted.

if (securityManager != null) {
securityManager.checkDelete(f.getAbsolutePath());
}
FILES_TO_DELETE_ON_SHUTDOWN.add(f);

This comment has been minimized.

Copy link
@sgflt

sgflt Sep 6, 2019

I wonder how much memory this will allocate and make unavailable for application. In some angle of view it could be considered as a memory leak.

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 8, 2019

Author Contributor

This mimics how the JDK does it when File.deleteOnExit() is called. Except for the fact that the JDK holds a static list of Strings.

* Java Runtime. Non-empty directories will be deleted recursively.
* @param f the file to delete on JVM shutdown
*/
public static void deleteOnExitRecursively(File f) {

This comment has been minimized.

Copy link
@sgflt

sgflt Sep 6, 2019

Is it ok to delete recursively files that are not directories?

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 8, 2019

Author Contributor

Sure. The same way it is ok to iterate a list with one item. But I see the wording problem.

final File[] files = file.listFiles();
if (files != null) {
for (File f : files) {
delete(f);

This comment has been minimized.

Copy link
@sgflt

sgflt Sep 6, 2019

I cant see the actual deletion, this does just recursion?

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 8, 2019

Author Contributor

A few lines below there is the file.delete() call.

@dmatej

This comment has been minimized.

Copy link
Contributor

dmatej commented Sep 6, 2019

Both ways are unreliable - SIGKILL prevents execution.
I would prefer variant 6 with NIO2: https://www.baeldung.com/java-delete-directory
This is interesting too: https://stackoverflow.com/questions/1424736/making-sure-file-gets-deleted-on-jvm-exit

@@ -73,14 +75,13 @@ public ExplodedURLClassloader(File explodeTo) throws IOException {

public ExplodedURLClassloader() throws IOException {
super(new URL[0]);
File directory;
directory = File.createTempFile("payaramicro-rt", "tmp");
File directory = File.createTempFile("payaramicro-rt", "tmp");

This comment has been minimized.

Copy link
@dmatej

dmatej Sep 6, 2019

Contributor

This is why we need recursive deletion of files and directories ... ok

This comment has been minimized.

Copy link
@dmatej

dmatej Sep 6, 2019

Contributor

But ... the directory variable will be a regular file, not a directory. See Files.createTempDirectory

This comment has been minimized.

Copy link
@pzygielo

pzygielo Sep 6, 2019

Contributor

But ... the directory variable will be a regular file, not a directory. See Files.createTempDirectory

Note, that only generated name is used, as in next line

if (!directory.delete() || !directory.mkdir()) { // convert the file into a directory.
there is attemp to convert it to directory.

This comment has been minimized.

Copy link
@dmatej

dmatej Sep 6, 2019

Contributor

You're right ... I forgot. Just another argument to use NIO instead of old io... it simplifies many things.

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 8, 2019

Author Contributor

Sure, I can switch it to NIO. Recursion is a bit tricky to read.

…e. Using NIO Files.walk() to traverse file tree to delete.
@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Sep 8, 2019

@sgflt @dmatej I have made the requested changes. Please review. The general unreliability of the shutdown hook has not been addressed here. This would need some more advanced process handling.

@svendiedrichsen svendiedrichsen changed the title Replace usage of File.deleteOnExit() with manual recursive deletion Replace usage of File.deleteOnExit() with manual deletion on exit Sep 8, 2019
private static final class FileDeletionThread extends Thread {

private static final Logger LOG = Logger.getLogger(FileDeletionThread.class.getName());
private final Set<String> filesToDelete = new LinkedHashSet<>();

This comment has been minimized.

Copy link
@sgflt

sgflt Sep 9, 2019

Is it sure only one thread at a time will access field filesToDelete? If not, there is a race condition that may cause ConcurrentModificationException during clean up (line 580).

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 9, 2019

Author Contributor

Good point. I will address this.

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 9, 2019

Author Contributor

I have addressed it.

private void delete(File file) {
if (file.exists()) {
try {
Files.walk(file.toPath())

This comment has been minimized.

Copy link
@sgflt

sgflt Sep 9, 2019

The stream is weakly consistent. It does not freeze the
* file tree while iterating, so it may (or may not) reflect updates to
* the file tree that occur after returned from this method.

Temporary directory may still exist after clean up if a new file appeared during clean up phase.

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 9, 2019

Author Contributor

How would you like this to be addressed? IMO this is not a specific problem of this solution but will also happen if you work with file.listFiles() to determine the content of a directory.

This comment has been minimized.

Copy link
@sgflt

sgflt Sep 10, 2019

I agree that every attempt may fail with possible race conditions and deleteOnExit does job as "best effort". (Windows has open descriptor to deleted file, new files appearing during hookup processing, etc.)

I don't know ho much robust have this utility be. Maybe it is ok just admit the fact in some cases there will be left some temporary directory. Or maybe not and we should spend more time with thinking about solution.

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 10, 2019

Author Contributor

We could still improve by having the files where deletion failed retried several times.

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Sep 9, 2019

@sgflt I have fixed the concurrency problem. How do you think one could fix the weakly consistency problem. IMO there is no way to handle it within the payara process.

final Set<String> filesToIterate = filesToDelete;
filesToDelete = new LinkedHashSet<>();
for (String path : filesToIterate) {
delete(new File(path));

This comment has been minimized.

Copy link
@pzygielo

pzygielo Sep 9, 2019

Contributor

Could Files (instead of Strings) be stored at 570 and this construction skipped here?

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 9, 2019

Author Contributor

Sure. That was my first approach. I have changed it to String from File when @sgflt was concerned about the amount of memory this would consume and that this was a memory leak. Are you sure you want to go back to storing File instances? Besides, File is just a wrapper around a String. Its size is about 10 bytes larger than the String.

This comment has been minimized.

Copy link
@pzygielo

pzygielo Sep 9, 2019

Contributor

Sure. That was my first approach...

I missed that ...when @sgflt was concerned.. already about this case.

IMO keeping Files would be more clear.
I think you acted way too fast with 9e8cccf, as I'm 3rd party here 😁

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 9, 2019

Author Contributor

We will see. I'm also for using Files.

This comment has been minimized.

Copy link
@sgflt

sgflt Sep 10, 2019

Files are OK for me. I was initially thinking about static array that is now gone. :) (moved to class as a field wich looks better to me)

final Set<String> filesToIterate = filesToDelete;
filesToDelete = new LinkedHashSet<>();

I was able to reproduce ConcurrentModificationException.
Without sync there is possible following sequence of steps:

T1 loads S1
T2 loads S1
T2 creates S2
T2 iterates over S1
T1 adds element to iterated S1

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Sep 10, 2019

@sgflt Using a ConcurrentSkipListSet to hold the file references now to prevent ConcurrentModificationExceptions. This should work without blocking and the outer loop assures that any file added during the inner deletion loop gets deleted in a successive run.

…implementation into separate class.
@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Sep 10, 2019

@sgflt I have added code to retry failed file deletion to improve on the situation where files are added during the file walk. Extracted FileDeletionThread into separate class file.

@pdudits

This comment has been minimized.

Copy link
Contributor

pdudits commented Sep 12, 2019

jenkins test please

@@ -53,13 +53,18 @@
<name>Payara Micro Boot</name>
<description>Payara Micro Edition Boot</description>
<dependencies>
<dependency>
<groupId>org.glassfish.main.common</groupId>

This comment has been minimized.

Copy link
@pdudits

pdudits Sep 13, 2019

Contributor

The groupId have changed to fish.payara.server.internal.common.

I see you reduced the scope, I'm curious to see how that affects contents of payara-micro jar.

This comment has been minimized.

Copy link
@svendiedrichsen

svendiedrichsen Sep 13, 2019

Author Contributor

I don't think there is a difference. The size comparing the payara-micro to a different previous build is 75.51 MB (before on a different build) to 75.52 MB (on this branch).

@pdudits

This comment has been minimized.

Copy link
Contributor

pdudits commented Sep 13, 2019

Jenkins test please

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Sep 13, 2019

@pdudits Are there any tests included in the automatic build for the Payara Micro server?

@pdudits

This comment has been minimized.

Copy link
Contributor

pdudits commented Sep 13, 2019

No, but I'll inspect the resulting .jar now and will run some.

@pdudits

This comment has been minimized.

Copy link
Contributor

pdudits commented Sep 16, 2019

This doesn't clean up the directories for me on Windows (but that could be Windows thing). However I wonder why I'm not getting any classloading exceptions, as my understanding is, that ExplodedURLClassLoader is the one that loads FileUtils, and therefore should blow up at startup.

I'll look into this more once my pile of tasks thickens.

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Sep 16, 2019

If you look at the maven payara-micro dependency graph you will see that common-util is pulled in as transitive dependency via glassfish -> config-api -> common-util already. Thats why I could declare it with scope provided. BTW the dependency graph of payara-micro is huge.

@pdudits

This comment has been minimized.

Copy link
Contributor

pdudits commented Sep 16, 2019

It's not about dependency graph, it's about runtime classpath. payara-micro-boot prepares classloader of all modules, inclusive common-util, by expanding contents of MICRO-INF from the jar into that temp directory -- that's what ExplodedURLClassLoader does.

Therefore that classloader implementation has no way of accessing FileUtils. At least that's what theory says, I'm looking forward to figuring out what practice looks like ;)

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Sep 16, 2019

Another question that came to my mind: Should we delete files that couldn't be deleted on exit delete on startup? Meaning when FileUtil class is loaded. I'd only need to store the list of undeleteable files in a text file in the temp folder on exit and pick it up when the class is loaded. That happens IMO before the ExplodedURLClassLoader extracts its content. This should work around the Windows Problem with deleting files on exit.

@pdudits

This comment has been minimized.

Copy link
Contributor

pdudits commented Sep 17, 2019

Oh, I was testing with wrong binary! 🤣 The right one indeed fails along my expectations with:

java.lang.NoClassDefFoundError: com/sun/enterprise/util/io/FileUtils
        at fish.payara.micro.boot.loader.ExplodedURLClassloader.<init>(ExplodedURLClassloader.java:84)

There are no nice solutions to this one, but after it initializes, it could use reflection to call FileUtils. I'll try that.

@svendiedrichsen

This comment has been minimized.

Copy link
Contributor Author

svendiedrichsen commented Sep 17, 2019

Now I got the problem. Took some time.

@pdudits

This comment has been minimized.

Copy link
Contributor

pdudits commented Sep 17, 2019

jenkins test please

@pdudits

This comment has been minimized.

Copy link
Contributor

pdudits commented Sep 17, 2019

I pushed the fix for classloader.

As for deleting files on startup, I wouldn't take the feature that far. It might fix the the issue with windows locking .jar files, but it would cause troubles during concurrent start of multiple instances, e. g. when system with multiple micro instances boots. And loose files to delete when multiple instances shutdown.

@pdudits pdudits changed the title Replace usage of File.deleteOnExit() with manual deletion on exit PAYARA-4143 Replace usage of File.deleteOnExit() with manual deletion on exit Sep 18, 2019
@pdudits pdudits added this to the 5.194 milestone Sep 18, 2019
@pdudits pdudits merged commit b7af7a0 into payara:master Sep 19, 2019
58 checks passed
58 checks passed
Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
@svendiedrichsen svendiedrichsen deleted the svendiedrichsen:delete-on-exit-recursively branch Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.