RFR: JDK-8225507: (zipfs): No space left on device not thrown with ZipFileSystem

[Jaikiran Pai]

Hello Alan,

On 09/02/20 10:32 pm, Alan Bateman wrote:
> On 05/02/2020 23:34, Lance Andersen wrote:
>> Hi Jaikiran,
>>
>> I think overall the patch is correct but it would be good to get some
>> extra eyes on this.
> On the surface this looks okay but I think it would be useful if you
> (or Jai) could write a brief summary about the state of the file
> system and the underlying channel when the sync fails. I think the zip
> file system state will be closed (!isOpen) so a later attempt to close
> it will be a no-op. However, the state of the channel is not clear -
> is it open or closed?

I hadn't paid attention to this aspect previously. I checked it now. If
an IOException occurs in sync(), before the channel.close() is attempted
within the sync() method, then the current implementation of
ZipFileSystem.close() leaves the channel open. This can happen even in
the current code where the IOException is caught and swallowed in a
couple of places, since there are other places within sync() which can
propagate an IOException.

It's not just that - the current implementation and even the proposed
IOException propagation in this patch can end up leaving the current
ZipFileSystem instance within the ZipFileSystemProvider#filesystems Map,
since the call to provider.removeFileSystem(zfpath, this) in the close()
method of ZipFileSystem ends up being skipped if the internal call to
sync() ends up throwing an IOException.

I don't have much knowledge of these classes, but I don't think this is
a good thing, because any subsequent call to the
ZipFileSystemProvider#newFileSystem(URI ...) with the exact same URI as
the previous one can result in a FileSystemAlreadyExistsException and
any calls to ZipFileSystemProvider#getFileSystem(URI ...) will result in
returning a closed instance of the filesystem.

Given that ZipFileSystemProvider is a capable of recreating new
filesystems using the same URI, the current implementation in the
ZipFileSystem#close() method would end up in a state where the
ZipFileSystemProvider would violate the contract of the
FileSystems#getFileSystem(URI...):

     * Once a file system created by this provider is {@link
FileSystem#close
     * closed} it is provider-dependent if this method returns a
reference to
     * the closed file system or throws {@link FileSystemNotFoundException}.
     * If the provider allows a new file system to be created with the
same URI
     * as a file system it previously created then this method throws the
     * exception if invoked after the file system is closed (and before
a new
     * instance is created by the {@link #newFileSystem newFileSystem}
method).

Again, as I noted, I am not too familiar with this piece of code (in
fact, having now looked at the jdk.nio.zipfs.ZipFileSystemProvider
implementation, I am surprised that the newFileSystem implementation
behaves completely differently when passed a URI as compared to when
passed a Path), so I may be overstating the potential issue.

-Jaikiran