8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

https://bugs.openjdk.java.net/browse/JDK-8193072 


There does appear to be a memory leak of sorts if one does something like

—

File[] files;
for (int i = 0; i < largeNumber; i++) {
files[i] = File.createTempFile(“blah”, null);
files[i].deleteOnExit();
}

// do something

for (int i = 0; i < largeNumber; i++) {
files[i].delete();
}

// do something else before shutdown

—

The LinkedHashSet in DeleteOnExitHook will contain at least largeNumber Files 
until the VM shuts down even though the files were deleted.

The potential change is included below. The additional call to 
DeleteOnExitHook.remove() in File.delete() does not appear to have a measurable 
performance impact, at least trivially and in isolation.

Thanks,

Brian

--- a/src/java.base/share/classes/java/io/DeleteOnExitHook.java
+++ b/src/java.base/share/classes/java/io/DeleteOnExitHook.java
@@ -64,6 +64,15 @@
 files.add(file);
 }
 
+static synchronized void remove(String file) {
+if(files == null) {
+// DeleteOnExitHook is running. Too late to remove a file
+throw new IllegalStateException("Shutdown in progress");
+}
+
+files.remove(file);
+}
+
 static void runHooks() {
 LinkedHashSet theFiles;
 
--- a/src/java.base/share/classes/java/io/File.java
+++ b/src/java.base/share/classes/java/io/File.java
@@ -1050,6 +1050,7 @@
 if (isInvalid()) {
 return false;
 }
+DeleteOnExitHook.remove(path);
 return fs.delete(this);
 }

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Lance Andersen]

Looks OK brian and does not seem to require any update to runHooks()  so all 
good for me :-)

> On Jul 8, 2019, at 4:11 PM, Brian Burkhalter  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8193072 
> 
> 
> There does appear to be a memory leak of sorts if one does something like
> 
> —
> 
> File[] files;
> for (int i = 0; i < largeNumber; i++) {
>files[i] = File.createTempFile(“blah”, null);
>files[i].deleteOnExit();
> }
> 
> // do something
> 
> for (int i = 0; i < largeNumber; i++) {
>files[i].delete();
> }
> 
> // do something else before shutdown
> 
> —
> 
> The LinkedHashSet in DeleteOnExitHook will contain at least largeNumber Files 
> until the VM shuts down even though the files were deleted.
> 
> The potential change is included below. The additional call to 
> DeleteOnExitHook.remove() in File.delete() does not appear to have a 
> measurable performance impact, at least trivially and in isolation.
> 
> Thanks,
> 
> Brian
> 
> --- a/src/java.base/share/classes/java/io/DeleteOnExitHook.java
> +++ b/src/java.base/share/classes/java/io/DeleteOnExitHook.java
> @@ -64,6 +64,15 @@
> files.add(file);
> }
> 
> +static synchronized void remove(String file) {
> +if(files == null) {
> +// DeleteOnExitHook is running. Too late to remove a file
> +throw new IllegalStateException("Shutdown in progress");
> +}
> +
> +files.remove(file);
> +}
> +
> static void runHooks() {
> LinkedHashSet theFiles;
> 
> --- a/src/java.base/share/classes/java/io/File.java
> +++ b/src/java.base/share/classes/java/io/File.java
> @@ -1050,6 +1050,7 @@
> if (isInvalid()) {
> return false;
> }
> +DeleteOnExitHook.remove(path);
> return fs.delete(this);
> }
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Ivan Gerasimov]

Hi Brian!

I believe this would introduce a behavior change in a scenario lile:
File f = ...;
f.deleteOnExit();
f.delete();
f.createNewFile();

I.e. when the with the same name is recreated.  Current behavior is to 
unlink such a file before exiting, no matter if it were explicitly 
deleted and then recreated or not.


With kind regards,
Ivan


On 7/8/19 1:11 PM, Brian Burkhalter wrote:

https://bugs.openjdk.java.net/browse/JDK-8193072 


There does appear to be a memory leak of sorts if one does something like

—

File[] files;
for (int i = 0; i < largeNumber; i++) {
 files[i] = File.createTempFile(“blah”, null);
 files[i].deleteOnExit();
}

// do something

for (int i = 0; i < largeNumber; i++) {
 files[i].delete();
}

// do something else before shutdown

—

The LinkedHashSet in DeleteOnExitHook will contain at least largeNumber Files 
until the VM shuts down even though the files were deleted.

The potential change is included below. The additional call to 
DeleteOnExitHook.remove() in File.delete() does not appear to have a measurable 
performance impact, at least trivially and in isolation.

Thanks,

Brian

--- a/src/java.base/share/classes/java/io/DeleteOnExitHook.java
+++ b/src/java.base/share/classes/java/io/DeleteOnExitHook.java
@@ -64,6 +64,15 @@
  files.add(file);
  }
  
+static synchronized void remove(String file) {

+if(files == null) {
+// DeleteOnExitHook is running. Too late to remove a file
+throw new IllegalStateException("Shutdown in progress");
+}
+
+files.remove(file);
+}
+
  static void runHooks() {
  LinkedHashSet theFiles;
  
--- a/src/java.base/share/classes/java/io/File.java

+++ b/src/java.base/share/classes/java/io/File.java
@@ -1050,6 +1050,7 @@
  if (isInvalid()) {
  return false;
  }
+DeleteOnExitHook.remove(path);
  return fs.delete(this);
  }



--
With kind regards,
Ivan Gerasimov

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Brian,

Previously File.delete wouldn't throw IllegalStateException and with this patch 
it looks like that is possible (and not desirable).  I would think that this 
change could the break java.util.logging.FileHandler because Handler.close runs 
in a shutdown hook.

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Monday, July 8, 2019 3:11 PM
To: core-libs-dev
Subject: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files

https://bugs.openjdk.java.net/browse/JDK-8193072 
<https://bugs.openjdk.java.net/browse/JDK-8193072>

There does appear to be a memory leak of sorts if one does something like

—

File[] files;
for (int i = 0; i < largeNumber; i++) {
files[i] = File.createTempFile(“blah”, null);
files[i].deleteOnExit();
}

// do something

for (int i = 0; i < largeNumber; i++) {
files[i].delete();
}

// do something else before shutdown

—

The LinkedHashSet in DeleteOnExitHook will contain at least largeNumber Files 
until the VM shuts down even though the files were deleted.

The potential change is included below. The additional call to 
DeleteOnExitHook.remove() in File.delete() does not appear to have a measurable 
performance impact, at least trivially and in isolation.

Thanks,

Brian

--- a/src/java.base/share/classes/java/io/DeleteOnExitHook.java
+++ b/src/java.base/share/classes/java/io/DeleteOnExitHook.java
@@ -64,6 +64,15 @@
 files.add(file);
 }

+static synchronized void remove(String file) {
+if(files == null) {
+// DeleteOnExitHook is running. Too late to remove a file
+throw new IllegalStateException("Shutdown in progress");
+}
+
+files.remove(file);
+}
+
 static void runHooks() {
 LinkedHashSet theFiles;

--- a/src/java.base/share/classes/java/io/File.java
+++ b/src/java.base/share/classes/java/io/File.java
@@ -1050,6 +1050,7 @@
 if (isInvalid()) {
 return false;
 }
+DeleteOnExitHook.remove(path);
 return fs.delete(this);
 }

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

Ivan / Jason,

Thanks for the good observations.

> On Jul 8, 2019, at 1:35 PM, Ivan Gerasimov  wrote:
> 
> I believe this would introduce a behavior change in a scenario lile:
> File f = ...;
> f.deleteOnExit();
> f.delete();
> f.createNewFile();
> 
> I.e. when the with the same name is recreated.  Current behavior is to unlink 
> such a file before exiting, no matter if it were explicitly deleted and then 
> recreated or not.

Good point. Given this consideration I am not sure that this bug can be fixed.

> On Jul 8, 2019, at 1:53 PM, Jason Mehrens  wrote:
> 
> Previously File.delete wouldn't throw IllegalStateException and with this 
> patch it looks like that is possible (and not desirable).  I would think that 
> this change could the break java.util.logging.FileHandler because 
> Handler.close runs in a shutdown hook.


I think you are correct that the ISE should not be thrown.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Roger Riggs]

Hi Brian,

The sequence described is the specified behavior of the API, whether it 
is a developer mistake or not is unknowable but it would be a 
compatibility issue to change it. The filename is the key and there is 
no way to determine if it is the original file or a replacement. 
deleteOnExit Wins!


$.02, Roger


On 7/8/19 5:08 PM, Brian Burkhalter wrote:

Ivan / Jason,

Thanks for the good observations.


On Jul 8, 2019, at 1:35 PM, Ivan Gerasimov  wrote:

I believe this would introduce a behavior change in a scenario lile:
File f = ...;
f.deleteOnExit();
f.delete();
f.createNewFile();

I.e. when the with the same name is recreated.  Current behavior is to unlink 
such a file before exiting, no matter if it were explicitly deleted and then 
recreated or not.

Good point. Given this consideration I am not sure that this bug can be fixed.


On Jul 8, 2019, at 1:53 PM, Jason Mehrens  wrote:

Previously File.delete wouldn't throw IllegalStateException and with this patch 
it looks like that is possible (and not desirable).  I would think that this 
change could the break java.util.logging.FileHandler because Handler.close runs 
in a shutdown hook.


I think you are correct that the ISE should not be thrown.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

Hi Roger,

You might be correct but I wrote up a different version at the end of 
yesterday. Not sure it is right but I might as well post it:

http://cr.openjdk.java.net/~bpb/8193072/webrev.01/

Thanks,

Brian

> On Jul 9, 2019, at 7:34 AM, Roger Riggs  wrote:
> 
> The sequence described is the specified behavior of the API, whether it is a 
> developer mistake or not is unknowable but it would be a compatibility issue 
> to change it. The filename is the key and there is no way to determine if it 
> is the original file or a replacement. deleteOnExit Wins!

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Roger Riggs]

Hi Brian,

The interesting part will be writing/updating the specification to make 
it clear what happens and under what conditions.

How often are File instances re-used vs creating new ones.
And any interactions with other APIs that create or delete files with 
the same name.  (file channels, zip, etc...)


Since deleteOnExit() is an deliberate act, perhaps there should be a 
corresponding withdrawDeleteOnExit() that reverses it so that it is 
intentional and not a side-effect of other File methods.


Roger


On 7/9/19 11:07 AM, Brian Burkhalter wrote:

Hi Roger,

You might be correct but I wrote up a different version at the end of 
yesterday. Not sure it is right but I might as well post it:


http://cr.openjdk.java.net/~bpb/8193072/webrev.01/

Thanks,

Brian

On Jul 9, 2019, at 7:34 AM, Roger Riggs > wrote:


The sequence described is the specified behavior of the API, whether 
it is a developer mistake or not is unknowable but it would be a 
compatibility issue to change it. The filename is the key and there 
is no way to determine if it is the original file or a replacement. 
deleteOnExit Wins!

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

Hi Roger,

> On Jul 9, 2019, at 8:25 AM, Roger Riggs  wrote:
> 
> The interesting part will be writing/updating the specification to make it 
> clear what happens and under what conditions.
> How often are File instances re-used vs creating new ones.
> And any interactions with other APIs that create or delete files with the 
> same name.  (file channels, zip, etc…)

These interactions also occurred to me. They would be impossible to account for.

> Since deleteOnExit() is an deliberate act, perhaps there should be a 
> corresponding withdrawDeleteOnExit() that reverses it so that it is 
> intentional and not a side-effect of other File methods.

I think this is a better idea. Perhaps “cancelDeleteOnExit()”.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

> On Jul 9, 2019, at 8:31 AM, Brian Burkhalter  
> wrote:
> 
>> Since deleteOnExit() is an deliberate act, perhaps there should be a 
>> corresponding withdrawDeleteOnExit() that reverses it so that it is 
>> intentional and not a side-effect of other File methods.
> 
> I think this is a better idea. Perhaps “cancelDeleteOnExit()”.

If we want to go this route, here is one possibility:

http://cr.openjdk.java.net/~bpb/8193072/webrev.02/

Of course a CSR would be called for if this is agreed upon.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Brian,

Just a note, one issue I see with webrev.01 is that JDK-7092892 is not 
resolved.  On one hand more calls to DeleteOnExitHook should trigger class init 
sooner avoiding the issue.  On the other it seems this could be more methods 
that could fail by throwing ExceptionInInitializerError that wouldn't have 
before the change.  I would think that you would want to mark JDK-7092892 as 
blocker of JDK-8193072 before you go this route.

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Tuesday, July 9, 2019 10:07 AM
To: Roger Riggs
Cc: core-libs-dev@openjdk.java.net
Subject: Re: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files

Hi Roger,

You might be correct but I wrote up a different version at the end of 
yesterday. Not sure it is right but I might as well post it:

http://cr.openjdk.java.net/~bpb/8193072/webrev.01/

Thanks,

Brian

> On Jul 9, 2019, at 7:34 AM, Roger Riggs  wrote:
>
> The sequence described is the specified behavior of the API, whether it is a 
> developer mistake or not is unknowable but it would be a compatibility issue 
> to change it. The filename is the key and there is no way to determine if it 
> is the original file or a replacement. deleteOnExit Wins!

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Would the SecurityManager need to for permissions (checkWrite or some new 
permission) before cancelDeleteOnExit() is allowed?

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Tuesday, July 9, 2019 1:08 PM
To: core-libs-dev
Subject: Re: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files


> On Jul 9, 2019, at 8:31 AM, Brian Burkhalter  
> wrote:
>
>> Since deleteOnExit() is an deliberate act, perhaps there should be a 
>> corresponding withdrawDeleteOnExit() that reverses it so that it is 
>> intentional and not a side-effect of other File methods.
>
> I think this is a better idea. Perhaps “cancelDeleteOnExit()”.

If we want to go this route, here is one possibility:

http://cr.openjdk.java.net/~bpb/8193072/webrev.02/

Of course a CSR would be called for if this is agreed upon.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

I don’t know. On the one hand this does not take an action like reading, 
writing, or deleting, but on the other it could end up causing files to be left 
lying around after VM termination which were expected to be deleted. I suppose 
that could be considered to be some sort of security issue.


Thanks,

Brian

> On Jul 9, 2019, at 2:26 PM, Jason Mehrens  wrote:
> 
> Would the SecurityManager need to for permissions (checkWrite or some new 
> permission) before cancelDeleteOnExit() is allowed?

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Peter Levart]

Hi,

On 7/9/19 8:08 PM, Brian Burkhalter wrote:

On Jul 9, 2019, at 8:31 AM, Brian Burkhalter  
wrote:


Since deleteOnExit() is an deliberate act, perhaps there should be a 
corresponding withdrawDeleteOnExit() that reverses it so that it is intentional 
and not a side-effect of other File methods.

I think this is a better idea. Perhaps “cancelDeleteOnExit()”.

If we want to go this route, here is one possibility:

http://cr.openjdk.java.net/~bpb/8193072/webrev.02/




With only one method (deleteOnExit) there are no races because the 
method is idempotent if called with the same File multiple times from 
different threads. Adding cancelDeleteOnExit() makes things problematic 
in concurrent setting. Imagine the following code:


    File f = new File("/path/to/file");

        // 1st register hook...
    f.deleteOnExit();
    // ...then attempt file creation so there's no chance
    // the file is left behind if VM unexpectedly exits
    if (f.createNewFile()) {
    ...
            ... process something using f
    ...
    // 1st delete file...
    f.delete();
    }
    // ...then unregister hook so there's no chance
    // the file is left behind if VM unexpectedly exits
    // unregister hook also after we registered it but
    // then file creation failed.
    f.undoDeleteOnExit();

This code is correct if executed in a single thread. But imagine two or 
more threads competing to create the same file and properly delete it 
afterwards with registering and un-registering the hook to cover cleanup 
when VM exits during processing...


There are various interleavings of threads that could cause the file to 
be left undeleted when VM exits.


To cover concurrent scenarios such as above, the code could use 
reference counting. Like in the following patch:


http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/

Regards, Peter

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Sean Mullan]

On 7/9/19 7:40 PM, Brian Burkhalter wrote:

I don’t know. On the one hand this does not take an action like reading, 
writing, or deleting, but on the other it could end up causing files to be left 
lying around after VM termination which were expected to be deleted. I suppose 
that could be considered to be some sort of security issue.


Yes I think so.

I would probably just use the same permission 
(FilePermission(file,"delete")). If you have been granted permission to 
delete a file, then you should have permission to cancel that deletion 
as well.


--Sean




Thanks,

Brian


On Jul 9, 2019, at 2:26 PM, Jason Mehrens  wrote:

Would the SecurityManager need to for permissions (checkWrite or some new 
permission) before cancelDeleteOnExit() is allowed?

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

Peter / Sean,

Thanks for the comments.

> On Jul 10, 2019, at 5:36 AM, Peter Levart  wrote:
> 
> There are various interleavings of threads that could cause the file to be 
> left undeleted when VM exits.
> 
> To cover concurrent scenarios such as above, the code could use reference 
> counting. Like in the following patch:
> 
> http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/
>  
> 
This looks good to me modulo adding this
SecurityManager security = System.getSecurityManager();
if (security != null) {
security.checkDelete(path);
}
to cancelDeleteOnExit() as suggested below.

> On Jul 10, 2019, at 7:51 AM, Sean Mullan  wrote:
> 
> On 7/9/19 7:40 PM, Brian Burkhalter wrote:
>> I don’t know. On the one hand this does not take an action like reading, 
>> writing, or deleting, but on the other it could end up causing files to be 
>> left lying around after VM termination which were expected to be deleted. I 
>> suppose that could be considered to be some sort of security issue.
> 
> Yes I think so.
> 
> I would probably just use the same permission 
> (FilePermission(file,"delete")). If you have been granted permission to 
> delete a file, then you should have permission to cancel that deletion as 
> well.

That’s a  good idea.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

I incorporated Peter’s version, adding the security check in 
cancelDeleteOnExit(), tweaking its verbiage along with that of deleteOnExit(), 
and modified the test DeleteOnExit to verify the new method. The updated 
version is here:

http://cr.openjdk.java.net/~bpb/8193072/webrev.03/ 


Thanks,

Brian

> On Jul 10, 2019, at 11:17 AM, Brian Burkhalter  
> wrote:
> 
> On Jul 10, 2019, at 5:36 AM, Peter Levart  wrote:
>> 
>> There are various interleavings of threads that could cause the file to be 
>> left undeleted when VM exits.
>> 
>> To cover concurrent scenarios such as above, the code could use reference 
>> counting. Like in the following patch:
>> 
>> http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/
>>  
>> 
> This looks good to me modulo adding this
>SecurityManager security = System.getSecurityManager();
>if (security != null) {
>security.checkDelete(path);
>}
> to cancelDeleteOnExit() as suggested below.
> 
>> On Jul 10, 2019, at 7:51 AM, Sean Mullan  wrote:
>> 
>> On 7/9/19 7:40 PM, Brian Burkhalter wrote:
>>> I don’t know. On the one hand this does not take an action like reading, 
>>> writing, or deleting, but on the other it could end up causing files to be 
>>> left lying around after VM termination which were expected to be deleted. I 
>>> suppose that could be considered to be some sort of security issue.
>> 
>> Yes I think so.
>> 
>> I would probably just use the same permission 
>> (FilePermission(file,"delete")). If you have been granted permission to 
>> delete a file, then you should have permission to cancel that deletion as 
>> well.
> 
> That’s a  good idea.

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Ivan Gerasimov]

On 7/10/19 5:17 PM, Brian Burkhalter wrote:

I incorporated Peter’s version, adding the security check in 
cancelDeleteOnExit(), tweaking its verbiage along with that of deleteOnExit(), 
and modified the test DeleteOnExit to verify the new method. The updated 
version is here:

http://cr.openjdk.java.net/~bpb/8193072/webrev.03/ 


There is possibility of a race here in a scenario like this:

File dir = new File("dir");
File file = new File("dir/file");

-- thread 1 --
dir.deleteOnExit();
file.deleteOnExit();
///
dir.cancelDeleteOnExit();
  thread 2 intervenes
file.cancelDeleteOnExit();
-- end --

-- thread 2 --
dir.deleteOnExit();
file.deleteOnExit();
-- end --

The result will be that the order of the registered files will change, 
and JVM will try to delete dir first (this will fail as it is not empty).


Of course it could be avoided, if cancellation were done in reverse 
order, though it's not immediately obvious from the documentation.


With kind regards,
Ivan

Thanks,

Brian


On Jul 10, 2019, at 11:17 AM, Brian Burkhalter  
wrote:

On Jul 10, 2019, at 5:36 AM, Peter Levart  wrote:

There are various interleavings of threads that could cause the file to be left 
undeleted when VM exits.

To cover concurrent scenarios such as above, the code could use reference 
counting. Like in the following patch:

http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/ 


This looks good to me modulo adding this
SecurityManager security = System.getSecurityManager();
if (security != null) {
security.checkDelete(path);
}
to cancelDeleteOnExit() as suggested below.


On Jul 10, 2019, at 7:51 AM, Sean Mullan  wrote:

On 7/9/19 7:40 PM, Brian Burkhalter wrote:

I don’t know. On the one hand this does not take an action like reading, 
writing, or deleting, but on the other it could end up causing files to be left 
lying around after VM termination which were expected to be deleted. I suppose 
that could be considered to be some sort of security issue.

Yes I think so.

I would probably just use the same permission (FilePermission(file,"delete")). 
If you have been granted permission to delete a file, then you should have permission to 
cancel that deletion as well.

That’s a  good idea.


--
With kind regards,
Ivan Gerasimov

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Peter Levart]

Hi,

On 7/11/19 3:51 AM, Ivan Gerasimov wrote:


On 7/10/19 5:17 PM, Brian Burkhalter wrote:
I incorporated Peter’s version, adding the security check in 
cancelDeleteOnExit(), tweaking its verbiage along with that of 
deleteOnExit(), and modified the test DeleteOnExit to verify the new 
method. The updated version is here:


http://cr.openjdk.java.net/~bpb/8193072/webrev.03/ 


There is possibility of a race here in a scenario like this:

File dir = new File("dir");
File file = new File("dir/file");

-- thread 1 --
dir.deleteOnExit();
file.deleteOnExit();
///
dir.cancelDeleteOnExit();
  thread 2 intervenes
file.cancelDeleteOnExit();
-- end --

-- thread 2 --
dir.deleteOnExit();
file.deleteOnExit();
-- end --

The result will be that the order of the registered files will change, 
and JVM will try to delete dir first (this will fail as it is not empty).


Of course it could be avoided, if cancellation were done in reverse 
order, though it's not immediately obvious from the documentation.


Hm,

LinkedHashMap.computeIfAbsent/computeIfPresent can also honor the so 
called "access order" which always moves the entry to the end of the 
linked list regardless of whether the entry is already present or not. 
So in above scenario and if LinkedHashMap was constructed with 
accessOrder=true, the registration order of paths after each operation 
would be as follows (the order of deletion is the reverse order of the 
presented registration order):


File dir = new File("dir");
File file = new File("dir/file");

-- thread 1 --

dir.deleteOnExit();
[dir]

file.deleteOnExit();
[dir, dir/file]

dir.cancelDeleteOnExit();
[dir/file]

-- thread 2 --

dir.deleteOnExit();
[dir/file, dir]

file.deleteOnExit();
[dir, dir/file]

-- thread 1 --

file.cancelDeleteOnExit();
[dir, dir/file]


But that is just coincidence. There are other interleavings which would 
cause LHM "access order" to reorder paths in undesired way. Perhaps the 
best behavior would be for deleteOnExit() to reorder the file to the end 
of the registration list while cancelDeleteOnExit() to not change the 
order of registered paths. For example, like this:


http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.02/

This does however change the order of registration for sequences like 
the following:


file1.deleteOnExit();
file2.deleteOnExit();
file1.deleteOnExit();

Order of deletion now: file2, file1
Order of deletion with this patch: file1, file2

But considering that programs that register multiple files do so 
consistently (always the same set of related files in one go in the same 
order) or register unrelated files in unimportant order, such behavior 
is perhaps acceptable.


What do you think?


Regards, Peter

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Peter Levart]

On 7/11/19 9:47 AM, Peter Levart wrote:


http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.02/ 





Another thing to consider (done in above webrev.02) is what to do with 
unbalanced cancelDeleteOnExit(). I think it is better to throw exception 
than to silently ignore it. This way unintentional bugs can be uncovered 
which would otherwise just cause erratic behavior.


Regards, Peter

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Would it work to fix this by making DeleteOnExitHook::runHooks deal with 
dependencies?
1. Remove If deleted, or not directory which also takes care of not exists.
2. Sort remaining files by deepest child files/directories first.
3. Run delete again on the list.

Otherwise files need to be processed in reverse order before directories and 
directories need to be processed children first up to the root.

Jason


From: core-libs-dev  on behalf of Ivan 
Gerasimov 
Sent: Wednesday, July 10, 2019 8:51 PM
To: Brian Burkhalter; core-libs-dev
Subject: Re: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files


On 7/10/19 5:17 PM, Brian Burkhalter wrote:
> I incorporated Peter’s version, adding the security check in 
> cancelDeleteOnExit(), tweaking its verbiage along with that of 
> deleteOnExit(), and modified the test DeleteOnExit to verify the new method. 
> The updated version is here:
>
> http://cr.openjdk.java.net/~bpb/8193072/webrev.03/ 
> <http://cr.openjdk.java.net/~bpb/8193072/webrev.03/>
There is possibility of a race here in a scenario like this:

File dir = new File("dir");
File file = new File("dir/file");

-- thread 1 --
dir.deleteOnExit();
file.deleteOnExit();
///
dir.cancelDeleteOnExit();
  thread 2 intervenes
file.cancelDeleteOnExit();
-- end --

-- thread 2 --
dir.deleteOnExit();
file.deleteOnExit();
-- end --

The result will be that the order of the registered files will change,
and JVM will try to delete dir first (this will fail as it is not empty).

Of course it could be avoided, if cancellation were done in reverse
order, though it's not immediately obvious from the documentation.

With kind regards,
Ivan
> Thanks,
>
> Brian
>
>> On Jul 10, 2019, at 11:17 AM, Brian Burkhalter  
>> wrote:
>>
>> On Jul 10, 2019, at 5:36 AM, Peter Levart  wrote:
>>> There are various interleavings of threads that could cause the file to be 
>>> left undeleted when VM exits.
>>>
>>> To cover concurrent scenarios such as above, the code could use reference 
>>> counting. Like in the following patch:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/
>>>  
>>> <http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/>
>> This looks good to me modulo adding this
>> SecurityManager security = System.getSecurityManager();
>> if (security != null) {
>> security.checkDelete(path);
>> }
>> to cancelDeleteOnExit() as suggested below.
>>
>>> On Jul 10, 2019, at 7:51 AM, Sean Mullan  wrote:
>>>
>>> On 7/9/19 7:40 PM, Brian Burkhalter wrote:
>>>> I don’t know. On the one hand this does not take an action like reading, 
>>>> writing, or deleting, but on the other it could end up causing files to be 
>>>> left lying around after VM termination which were expected to be deleted. 
>>>> I suppose that could be considered to be some sort of security issue.
>>> Yes I think so.
>>>
>>> I would probably just use the same permission 
>>> (FilePermission(file,"delete")). If you have been granted permission to 
>>> delete a file, then you should have permission to cancel that deletion as 
>>> well.
>> That’s a  good idea.

--
With kind regards,
Ivan Gerasimov

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

> On Jul 11, 2019, at 12:52 AM, Peter Levart  wrote:
> 
> On 7/11/19 9:47 AM, Peter Levart wrote:
>> 
>> http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.02/
>>  
>> 
>>  
>> 
> 
> Another thing to consider (done in above webrev.02) is what to do with 
> unbalanced cancelDeleteOnExit(). I think it is better to throw exception than 
> to silently ignore it. This way unintentional bugs can be uncovered which 
> would otherwise just cause erratic behavior.

The above patch looks pretty good but unless I am not comprehending the code I 
think there may still be behavioral incompatibilities which might not be 
acceptable. For example, for the existing code base with the following sequence 
of operations

File file1, file2, file3;

file1.deleteOnExit();
file2.deleteOnExit();
file3.deleteOnExit();

the deletion order is file3, file2, file1. For the proposed patch with the 
following sequence of operations

file1.deleteOnExit();
file2.deleteOnExit();
file3.deleteOnExit();
file1.cancelDeleteOnExit(); // file1 is removed from the map
file2.cancelDeleteOnExit(); // file2 is removed from the map
file2.createNewFIle();
file2.deleteOnExit(); // file2 is added back into the map
file1.createNewFIle();
file1.deleteOnExit(); // file1 is added back into the map

the deletion order is (I think) file1, file2, file3 which is the reverse of the 
order of initial registration. Of course it is conceivable to change the 
specification but that seems dangerous somehow. Also for the patch above for 
this sequence of operations

file1.deleteOnExit();
file2.deleteOnExit();
file3.deleteOnExit();
file1.cancelDeleteOnExit(); // file1 is removed from the map
file2.cancelDeleteOnExit(); // file2 is removed from the map
file2.createNewFIle();
file1.createNewFIle();

then file3 is deleted on exit but file1 and file2 are not which differs from 
current behavior.

As Roger wrote

> On Jul 9, 2019, at 7:34 AM, Roger Riggs  wrote:
> 
> The filename is the key and there is no way to determine if it is the 
> original file or a replacement. deleteOnExit Wins!

the filename is the key and if we toss the key then we lose the order of 
registration. Given this I am not sure any more that it is possible to fix this 
issue without introducing an incompatible behavioral change.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

> On Jul 25, 2019, at 3:42 PM, Brian Burkhalter  
> wrote:
> 
> the deletion order is (I think) file1, file2, file3 which is the reverse of 
> the order of initial registration.

I intended *not* the reverse order of initial registration.

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Peter Levart]

Hi Brian,

On 7/26/19 12:42 AM, Brian Burkhalter wrote:


On Jul 11, 2019, at 12:52 AM, Peter Levart > wrote:


On 7/11/19 9:47 AM, Peter Levart wrote:


http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.02/



Another thing to consider (done in above webrev.02) is what to do 
with unbalanced cancelDeleteOnExit(). I think it is better to throw 
exception than to silently ignore it. This way unintentional bugs can 
be uncovered which would otherwise just cause erratic behavior.


The above patch looks pretty good but unless I am not comprehending 
the code I think there may still be behavioral incompatibilities which 
might not be acceptable. For example, for the existing code base with 
the following sequence of operations


File file1, file2, file3;

file1.deleteOnExit();
file2.deleteOnExit();
file3.deleteOnExit();

the deletion order is file3, file2, file1.


...and the same order remains with the proposed patch for above sequence 
of operations.




For the proposed patch with the following sequence of operations

file1.deleteOnExit();
file2.deleteOnExit();
file3.deleteOnExit();
file1.cancelDeleteOnExit(); // file1 is removed from the map
file2.cancelDeleteOnExit(); // file2 is removed from the map
file2.createNewFIle();
file2.deleteOnExit(); // file2 is added back into the map
file1.createNewFIle();
file1.deleteOnExit(); // file1 is added back into the map

the deletion order is (I think) file1, file2, file3 which is the 
reverse of the order of initial registration.


Right, and above sequence of operations is something that is not present 
in current codebases as there is no cancelDeleteOnExit() method yet. So 
more to the point would be a comparison of behaviors with some code 
sequence that doesn't include the not yet present method. Suppose the 
following:


file1.deleteOnExit();
file1.createNewFile();
file2.deleteOnExit();
file2.createNewFile();
file3.deleteOnExit();
file3.createNewFile();

file1.delete();
file2.delete();

file2.deleteOnExit();
file2.createNewFile();
file1.deleteOnExit();
file1.createNewFile();

Yes, with above sequence the order of deletion using current codebase 
will be file3, file2, file1, while with the patch, it would be file1, 
file2, file3.



Of course it is conceivable to change the specification but that seems 
dangerous somehow.


Of course there could be a situation where the change of order mattered 
for the correct logic of the program - imagine an empty "signal" file 
which guarantees with its presence that another "payload" file is 
present and fully written/consistent. You would want to maintain such an 
invariant so you would 1st register the payloadFile.deleteOnExit() 
following with signalFile.deleteOnExit(). But most such schemes are 
one-way only. If you wanted to somehow delete the signal file and then 
re-create it later after updating the payload file, you might already be 
doing the re-registration in the correct order (if you are just 
repeating the same code sequence as in initial registration), but you 
are making a concurrency mistake anyway as you are faced with a race 
inherent to an ABA problem that has nothing to do with registration to 
delete files on exit.


So I argue that any working scheme that features multiple registrations 
of the same file must be because of dependencies among them stemming 
from the filesystem hierarchy. For example, you would 1st want to 
register a new File("/path/to/dir").deleteOnExit() followed by new 
File("/path/to/dir/file.ext").deleteOnExit() so that on exit, the file 
is deleted 1st and then the dir which must be empty for deletion to succeed.


In this hypothetical example, you might, at some point delete the file 
and unregister it (keeping the dir present and registered) and later 
re-register and re-create the file. The deletion will still work 
correctly in this case. Or you might want to delete the file and dir and 
unregister them for them to be re-registered and re-created later. If 
such code is present now (modulo de-registering) it probably performs 
the re-registration in the correct order already.


In other words, any code that changes the deletion order with the patch 
but doesn't with current codebase is re-registering the same file (in 
wrong order) relying on the fact that the file is already registered (in 
the correct order). Only such code could break, but the chances are 
great the it won't.




Also for the patch above for this sequence of operations

file1.deleteOnExit();
file2.deleteOnExit();
file3.deleteOnExit();
file1.cancelDeleteOnExit(); // file1 is removed from the map
file2.cancelDeleteOnExit(); // file2 is removed from the map
file2.createNewFIle();
file1.createNewFIle();

then file3 is deleted on exit but file1 and file2 are not which 
differs from current behavior.



Right, because there is "no current behavior" with above sequence of 
operations, because there is currently no cancelDeleteOnExit() method. 
So the