RFR: 8280881: (fs) UnixNativeDispatcher.close0 may throw UnixException

[Brian Burkhalter]

On Tue, 15 Feb 2022 04:25:32 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Re-specify `sun.nio.fs.UnixNativeDispatcher.close0()` to throw `sun.nio.fs.UnixException` and modify callers either to ignore it or to rethrow it as an `IOException`, as appropriate.
>
> src/java.base/unix/classes/sun/nio/fs/UnixFileSystemProvider.java line 438:
> 
>> 436:                     UnixNativeDispatcher.close(dfd1);
>> 437:                 } catch (UnixException y) {
>> 438:                     x.addSuppressed(y);
> 
> Hello Brian,
> I think, even after adding this as suppressed exception, this (suprressed) exception will be "lost" because at the end of this outer catch block there's this code:
> 
> x.rethrowAsIOException(dir);
> 
> and the implementation of `rethrowAsIOException` internally just creates a new exception instance (depending on the error code) but doesn't capture any suppressed exceptions from the original `x`.

Good point.

> src/java.base/unix/classes/sun/nio/fs/UnixSecureDirectoryStream.java line 128:
> 
>> 126:                         UnixNativeDispatcher.close(newdfd1);
>> 127:                     } catch (UnixException e) {
>> 128:                         x.addSuppressed(e);
> 
> Same here with respect to suppressed exception and what happens at the end of this catch block with `x.rethrowAsIOException(file);`

Ditto.

> src/java.base/unix/classes/sun/nio/fs/UnixUserDefinedFileAttributeView.java line 139:
> 
>> 137:                 close(fd);
>> 138:             } catch (UnixException y) {
>> 139:                 y.rethrowAsIOException(file);
> 
> Hello Brian,
> A few lines above this code in the finally block, there's a catch block which catches the `UnixException` and rethrows it as a `FileSystemException`. With the change being done in this finally block, this finally block now potentially can throw an `IOException` from a failed call to `close()` and thus the original `FileSystemException` can get lost. Do you think it would be better to keep track any potential `FileSystemException` from the previous catch block and add this `IOException` from this `close()` call as a suppressed exception to that `FileSystemException` and rethrow that original exception? Loosely, something like:
> 
> 
> IOException failure = null;
> try {
> 	....
> 
> } catch (UnixException x) {
> 	failure = new FileSystemException(file.getPathForExceptionMessage(),
> 	    null, "Unable to get list of extended attributes: " +
> 	    x.getMessage());
> } finally {
>     try {
> 	    close(fd);
> 	} catch (UnixException y) {
> 		if (failure == null) {
> 			// throw the exception that happened during close
> 			y.rethrowAsIOException(file);
> 		} else {
> 			// add the failure to close as a suppressed exception to
> 			// the previously caught failure
> 			failure.addSuppressed(y.asIOException(file));
> 		}
> 	}
> 	// throw any previously caught failure
> 	if (failure != null) {
> 		throw failure;
> 	}
> }
> 
> 
> This applies to few others places that have changed in this PR.

This might be a good idea. I think I need to revisit all the changes in this patch.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7467