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 <peter.levart at gmail.com 
>> <mailto:peter.levart at gmail.com>> 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 there's no valid comparison.


>
> As Roger wrote
>
>> On Jul 9, 2019, at 7:34 AM, Roger Riggs <Roger.Riggs at Oracle.com 
>> <mailto:Roger.Riggs at Oracle.com>> 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.


What above a system property that restores the previous behavior and is 
deprecated at start with removal to be determined?


Regards, Peter


>
> Thanks,
>
> Brian
>
>