Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8274883: (se) Selector.open throws IAE when the default file system provider is changed to a custom provider #6722

Closed
wants to merge 3 commits into from

Conversation

mkartashev
Copy link
Member

@mkartashev mkartashev commented Dec 6, 2021

The gist of the problem: when a file system is specified via -Djava.nio.file.spi.DefaultFileSystemProvider, a call to SelectorProvider.provider().openSelector() ends with a throw on Windows.

There are two distinct components to the problem:

  1. ExceptionInInitializerError is thrown during the static initialization of the UnixDomainSockets.UNNAMED field even though it isn't used on this code path (see UnixDomainSocketAddress.of() that throws IllegalArgumentException if invoked on a path from a non-default file system).
    This is fixed by lazy-initializing the static member UNNAMED of UnixDomainSockets so that this initialization doesn't throw unless actually used.

  2. IllegalArgumentException is thrown by UnixDomainSocketAddress.of() later on when ServerSocketChannel tries to use Windows version of PipeImpl and its method createListener() specifically. That PipeImpl probes for the availability of Unix Domain Sockets by trying to bind to a unique temporary name. That call throws IAE when a non-default Java file system is installed while the probing code (PipeImpl.createListener()) only expects UnsupportedOperationException or IOException.
    The fix is to re-throw UOE instead of IAE in UnixDomainSockets.genrateTempName(). This is more consistent with the definition of the exception purpose ("requested operation is not supported"). So with this change, a loopback network socket will be used to implement a pipe on a non-default Java file system. Also, pipes do not rely on the default Java file system on other platforms (Linux, MacOS) as well.

Tested by running jtreg:test/jdk/java/nio on Window, MacOS, and Linux.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8274883: (se) Selector.open throws IAE when the default file system provider is changed to a custom provider

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6722/head:pull/6722
$ git checkout pull/6722

Update a local copy of the PR:
$ git checkout pull/6722
$ git pull https://git.openjdk.java.net/jdk pull/6722/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6722

View PR using the GUI difftool:
$ git pr show -t 6722

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6722.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 6, 2021

👋 Welcome back mkartashev! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Dec 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 6, 2021

@mkartashev The following label will be automatically applied to this pull request:

  • nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the nio label Dec 6, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 6, 2021

Webrevs

Copy link
Contributor

@AlanBateman AlanBateman left a comment

The JBS issue is assigned to @Michael-Mc-Mahon, might need to check if he already has a patch this for too.

@@ -178,4 +182,8 @@ private static native int accept0(FileDescriptor fd, FileDescriptor newfd, Objec
IOUtil.load();
supported = init();
}

static UnixDomainSocketAddress getUNNAMED() {
Copy link
Contributor

@AlanBateman AlanBateman Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this to unnamed() and move it up with the other package default static methods?

Copy link
Member Author

@mkartashev mkartashev Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing!
Renamed to unnamed().

@@ -44,7 +44,9 @@
class UnixDomainSockets {
private UnixDomainSockets() { }

static final UnixDomainSocketAddress UNNAMED = UnixDomainSocketAddress.of("");
private static class UNNAMEDHolder {
Copy link
Contributor

@AlanBateman AlanBateman Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to UnamedHolder to avoid this strange class name.

Copy link
Member Author

@mkartashev mkartashev Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also renamed.

@@ -137,6 +139,8 @@ static UnixDomainSocketAddress generateTempName() throws IOException {
return UnixDomainSocketAddress.of(path);
} catch (InvalidPathException e) {
throw new BindException("Invalid temporary directory");
} catch (IllegalArgumentException e) {
throw new UnsupportedOperationException("Unix Domain Sockets not supported on non-default file system");
Copy link
Contributor

@AlanBateman AlanBateman Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no need to catch IAE here, instead you can check the provider.

Copy link
Member Author

@mkartashev mkartashev Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to if (path.getFileSystem().provider() != sun.nio.fs.DefaultFileSystemProvider.instance()) ...
I hope that was what you meant.

- renamed UNNAMEDHolder to UnnamedHolder,
- renamed getUNNAMED() to unnamed(),
- replaced catch IllegalArgumentException with a check for the provider.
@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Dec 7, 2021

@Michael-Mc-Mahon The issue is assigned to you? Are you okay if @mkartashev takes it?

The updated src changes looks okay.

test/jdk/java/nio/channels/Selector is probably the right location for the test as this issue is about the wakeup mechanism. I'd prefer to use a test name that is a bit more consistent with the naming in the area. Something like CustomFileSystem.java might be okay. We can also change the test to use Selector.open() as that is the user-facing API. I assume @library /test/lib can be dropped from the test description as the test doesn't use common test infrastructure.

Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon left a comment

Thanks for taking this on. I can sponsor it when we are finished with the review. Would it be possible to refer to the TestProvider in the SelectorProvider/spi directory in the @build directive, instead of copying it over to the Selector directory?

@mkartashev
Copy link
Member Author

@mkartashev mkartashev commented Dec 7, 2021

I can sponsor it when we are finished with the review.

Thank you, much appreciated!

Would it be possible to refer to the TestProvider in the SelectorProvider/spi directory in the @build directive, instead of copying it over to the Selector directory?

I looked for a way to do that, but couldn't find any. Besides, the new variant of the custom provider is much smaller and simpler than the "original" TestProvider and slightly better suits the purpose. So it's not a completely wasted effort (i.e. if things break in TestProvider for reasons unrelated to Unix domain sockets, the new test is less likely to break).

@Michael-Mc-Mahon
Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon commented Dec 7, 2021

I can sponsor it when we are finished with the review.

Thank you, much appreciated!

Would it be possible to refer to the TestProvider in the SelectorProvider/spi directory in the @build directive, instead of copying it over to the Selector directory?

I looked for a way to do that, but couldn't find any. Besides, the new variant of the custom provider is much smaller and simpler than the "original" TestProvider and slightly better suits the purpose. So it's not a completely wasted effort (i.e. if things break in TestProvider for reasons unrelated to Unix domain sockets, the new test is less likely to break).

That's okay with me. It's useful to have it in the same directory anyway. I'm just running the patch through the test system here now.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2021

@mkartashev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8274883: (se) Selector.open throws IAE when the default file system provider is changed to a custom provider

Reviewed-by: alanb, michaelm

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 33 new commits pushed to the master branch:

  • 7ea4b19: 8278166: java/nio/channels/Channels/TransferTo.java timed out
  • 543d1a8: 8275721: Name of UTC timezone in a locale changes depending on previous code
  • bd7c54a: 8278341: Liveness check for global scope is not as fast as it could be
  • c609b5d: 8277628: Spec for InetAddressResolverProvider::get() throwing error or exception could be clearer
  • bb50b92: 8277536: Use String.blank in jdk.javadoc where applicable
  • 5b81d5e: 8276901: Implement UseHeavyMonitors consistently
  • 69d8669: 8278339: ServerSocket::isClosed may return false after accept throws
  • 56ca66e: 8277863: Deprecate sun.misc.Unsafe methods that return offsets
  • 3536127: 8277383: VM.metaspace optionally show chunk freelist details
  • 44fcee3: 8278289: Drop G1BlockOffsetTablePart::_object_can_span
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/194cdf4e28225133dcdf29cf1bf4e580f3fd9208...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@AlanBateman, @Michael-Mc-Mahon) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Dec 7, 2021
@mkartashev
Copy link
Member Author

@mkartashev mkartashev commented Dec 7, 2021

/integrate

@openjdk openjdk bot added the sponsor label Dec 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2021

@mkartashev
Your change (at version 123cd5d) is now ready to be sponsored by a Committer.

Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon left a comment

Test results look normal apart from a couple of known intermittent issues

@Michael-Mc-Mahon
Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon commented Dec 7, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2021

Going to push as commit 7217cb7.
Since your change was applied there have been 33 commits pushed to the master branch:

  • 7ea4b19: 8278166: java/nio/channels/Channels/TransferTo.java timed out
  • 543d1a8: 8275721: Name of UTC timezone in a locale changes depending on previous code
  • bd7c54a: 8278341: Liveness check for global scope is not as fast as it could be
  • c609b5d: 8277628: Spec for InetAddressResolverProvider::get() throwing error or exception could be clearer
  • bb50b92: 8277536: Use String.blank in jdk.javadoc where applicable
  • 5b81d5e: 8276901: Implement UseHeavyMonitors consistently
  • 69d8669: 8278339: ServerSocket::isClosed may return false after accept throws
  • 56ca66e: 8277863: Deprecate sun.misc.Unsafe methods that return offsets
  • 3536127: 8277383: VM.metaspace optionally show chunk freelist details
  • 44fcee3: 8278289: Drop G1BlockOffsetTablePart::_object_can_span
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/194cdf4e28225133dcdf29cf1bf4e580f3fd9208...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 7, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Dec 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2021

@Michael-Mc-Mahon @mkartashev Pushed as commit 7217cb7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@mkartashev
Copy link
Member Author

@mkartashev mkartashev commented Dec 7, 2021

@Michael-Mc-Mahon @AlanBateman Thank you for your time!

@mkartashev mkartashev deleted the JDK-8274883 branch Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated nio
3 participants