Seeking inputs on 8224794: ZipFile/JarFile should open zip/JAR file with FILE_SHARE_DELETE sharing mode on Windows

[Jaikiran Pai]

Would there be interest in moving this forward? Enabling that 
FILE_SHARE_DELETE option has opened up some questions that I noted in my 
previous mail below. So in order to move forward I would need some 
inputs. If we don't want to do this at this time, I'll close the draft 
PR that's currently open https://github.com/openjdk/jdk/pull/6477.

-Jaikiran

On 19/11/21 5:10 pm, Jaikiran Pai wrote:
> I have been experimenting with 
> https://bugs.openjdk.java.net/browse/JDK-8224794 and have reached a 
> stage with my changes where I would like some inputs before I move 
> ahead or raise a PR for review.
>
> As noted in that JBS issue, the idea is to open the underlying file 
> using FILE_SHARE_DELETE when using java.uitl.zip.ZipFile and 
> java.util.jar.JarFile. From what I can infer from that JBS issue, the 
> motivation behind this seems to be to allow deleting that underlying 
> file when that file is currently opened and in use by the 
> ZipFile/JarFile instance. The linked github issues seem to suggest the 
> same. IMO, it's a reasonable expectation, given that such a deletion 
> works on *nix platform. It's on Windows where such a deletion fails 
> with "The process cannot access the file because it is being used by 
> another process" (the other process in many cases will be the current 
> JVM instance itself). So trying to get this to work on Windows by 
> setting the FILE_SHARE_DELETE share access mode seems reasonable.
>
> Coming to the implementation part, I've an initial draft version here 
> https://github.com/openjdk/jdk/pull/6477. It sets the 
> FILE_SHARE_DELETE share access mode on the file when constructed by 
> ZipFile/JarFile. It "works", in the sense that if you issue a delete 
> against this underlying file (path) after its been opened by 
> ZipFile/JarFile instance, you no longer see the error mentioned above 
> and the delete completes without any exception. However, my 
> experiments with this change have raised a bunch of questions related 
> to this:
>
> The crucial part of the "DeleteFile" API in Windows is that, as stated 
> in its documentation[1]:
>
> "The DeleteFile function marks a file for deletion on close. 
> Therefore, the file deletion does not occur until the last handle to 
> the file is closed. Subsequent calls to CreateFile to open the file 
> fail with ERROR_ACCESS_DENIED."
>
> So imagine the following pseudo-code:
>
> String path = "C:/foo.jar"
> 1. JarFile jar = new JarFile(path); // create a jar instance
> 2. var deleted = new File(path).delete(); // delete the underlying file
> 3. var exists = new File(path).exists(); // check file existence
> 4. FileOutputStream fos = new FileOutputStream(new File(path)); // 
> open a FileOutputStream for /tmp/foo.jar
> 5. java.nio.file.Files.deleteIfExists(Path.of(path));
>
> When this is run (on Windows) *without* the change that introduces 
> FILE_SHARE_DELETE, I notice that line#2 returns "false" for 
> File.delete() (since the file is in use by the JarFile instance) and 
> then line#3 returns "true" since it still exists and line#4 succeeds 
> and creates a new FileOutputStream and line#5 fails with an exception 
> stating "java.nio.file.FileSystemException: foo.jar: The process 
> cannot access the file because it is being used by another process
> "
>
> Now when the same code is run (on Windows) *with* the proposed 
> FILE_SHARE_DELETE changes, I notice that line#2 returns "true" (i.e. 
> the file is considered deleted) and line#3 returns "true" (i.e. it's 
> considered the file still exists). So essentially the file is only 
> marked for deletion and hasn't been deleted at the OS level, since the 
> JarFile instance still has an handle open. Then line#4 unlike 
> previously, now throws an exception and fails to create the 
> FileOutputStream instance. The failure says 
> "java.nio.file.AccessDeniedException: foo.jar". This exception matches 
> the documentation from that Windows DeleteFile API. line#5 too fails 
> with this same AccessDeniedException.
>
> So what this means is, although we have managed to allow the deletion 
> to happen (i.e. the first call to delete does mark it for deletion at 
> OS level), IMO, it still doesn't improve much and raises other 
> difference in behaviour of APIs as seen above, unless the underlying 
> file handle held by ZipFile/JarFile is closed. So that brings me to 
> the next related issue.
>
> For this change to be really useful, the file handle (which is an 
> instance of RandomAccessFile) held by ZipFile/JarFile needs to be 
> closed. If a user application explicitly creates an instance of 
> ZipFile/JarFile, it is expected that they close it themselves. 
> However, the most prominent use of the JarFile creation is within the 
> JDK code where it uses the sun.net.www.protocol.jar.JarURLConnection 
> to construct the JarFile instances, typically from the URLClassLoader. 
> By default these JarFile instances that are created by the 
> sun.net.www.protocol.jar.JarURLConnection are cached and the close() 
> on those instances is called only when the URLClassLoader is 
> close()ed. That in itself doesn't guarantee that the underlying file 
> descriptor corresponding to the JarFile instance gets closed, because 
> there's one more level of reference counting "cache" in use in 
> java.util.zip.ZipFile$Source class where the same underlying file 
> descriptor (corresponding to the jar file) gets used by multiple 
> JarFile instances (irrespective of who created those). So in short, 
> the underlying file descriptor of a JarFile only gets closed when 
> every single instance of a JarFile pointing to the same jar file has 
> been close()ed within the JVM. Which effectively means that 
> introducing the FILE_SHARE_DELETE semantics/changes for Windows will 
> only allow the first delete() attempt to succeed (since that one marks 
> it for deletion), but any subsequent operations like the one noted in 
> the examples, that rely on that file being deleted will continue to 
> fail with Access Denied errors. Given all this caching of JarFile and 
> the reference count cache for the underlying jar file, is this change 
> of FILE_SHARE_DELETE adding much value and should I pursue it further?
>
> [1] 
> https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew
>
> -Jaikiran
>