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

8280944: Enable Unix domain sockets in Windows Selector notification mechanism #7302

Closed
wants to merge 8 commits into from

Conversation

Michael-Mc-Mahon
Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon commented Feb 1, 2022

Hi,

Could I get the following change reviewed please?

8280233 temporarily disabled AF_UNIX sockets in the windows pipe implementation due to a Windows bug. We would like to re-enable one internal usage of AF_UNIX pipes in the JDK, for the windows NIO selector notification mechanism since this use case does not involve closing the socket and should therefore not encounter the bug.

I haven't included a regression test as this change will exercise tests that are currently running into TCP resource limitations on windows 10 client systems.

Thanks,
Michael


Progress

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

Issue

  • JDK-8280944: Enable Unix domain sockets in Windows Selector notification mechanism

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7302

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 1, 2022

👋 Welcome back michaelm! 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
Copy link

openjdk bot commented Feb 1, 2022

@Michael-Mc-Mahon 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 nio-dev@openjdk.org label Feb 1, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 1, 2022
@mlbridge
Copy link

mlbridge bot commented Feb 1, 2022

Webrevs

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

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

LGTM, but it would be good to have Brian B. or Alan have a look too.

@openjdk
Copy link

openjdk bot commented Feb 1, 2022

@Michael-Mc-Mahon 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:

8280944: Enable Unix domain sockets in Windows Selector notification mechanism

Reviewed-by: dfuchs, alanb

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 42 new commits pushed to the master branch:

  • 4ea6037: 8281035: Serial: Move RemoveForwardedPointerClosure to local scope
  • ae2504b: 8278254: Cleanup doclint warnings in java.desktop module
  • de826ba: 8280600: C2: assert(!had_error) failed: bad dominance
  • 4304a77: 8279535: C2: Dead code in PhaseIdealLoop::create_loop_nest after JDK-8276116
  • ab63834: 8280885: Shenandoah: Some tests failed with "EA: missing allocation reference path"
  • 48a32b5: 8280976: Incorrect encoding of avx512 vpsraq instruction with mask and constant shift.
  • 97af323: 8280842: Access violation in ciTypeFlow::profiled_count
  • d32f99e: 8279219: [REDO] C2 crash when allocating array of size too large
  • 85d839f: 8280601: ClhsdbThreadContext.java test is triggering codecache related asserts
  • 9ca7ff3: 8281082: Improve javadoc references to JOSS
  • ... and 32 more: https://git.openjdk.java.net/jdk/compare/251351f49498ea39150b38860b8f73232fbaf05d...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Feb 1, 2022
@AlanBateman
Copy link
Contributor

LGTM, but it would be good to have Brian B. or Alan have a look too.

I plan to review this.

if (preferUnixDomain && UnixDomainSockets.isSupported()) {
try {
listener = ServerSocketChannel.open(UNIX);
listener.bind(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Updating version looks okay, I just wondering if bind can fail, in which case it would leak a socket.

Copy link
Member Author

Choose a reason for hiding this comment

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

bind would only fail if it can't find a random unique name in the temp directory. Probability low, but not zero. It doesn't cost much to check if listener != null and close it.

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and the clarification that UOE is thrown when the default file system provider is not the built-in provider.

@Michael-Mc-Mahon
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Feb 2, 2022

Going to push as commit 87ab099.
Since your change was applied there have been 43 commits pushed to the master branch:

  • ce71e8b: 8279917: Refactor subclassAudits in Thread to use ClassValue
  • 4ea6037: 8281035: Serial: Move RemoveForwardedPointerClosure to local scope
  • ae2504b: 8278254: Cleanup doclint warnings in java.desktop module
  • de826ba: 8280600: C2: assert(!had_error) failed: bad dominance
  • 4304a77: 8279535: C2: Dead code in PhaseIdealLoop::create_loop_nest after JDK-8276116
  • ab63834: 8280885: Shenandoah: Some tests failed with "EA: missing allocation reference path"
  • 48a32b5: 8280976: Incorrect encoding of avx512 vpsraq instruction with mask and constant shift.
  • 97af323: 8280842: Access violation in ciTypeFlow::profiled_count
  • d32f99e: 8279219: [REDO] C2 crash when allocating array of size too large
  • 85d839f: 8280601: ClhsdbThreadContext.java test is triggering codecache related asserts
  • ... and 33 more: https://git.openjdk.java.net/jdk/compare/251351f49498ea39150b38860b8f73232fbaf05d...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 2, 2022
@openjdk openjdk bot closed this Feb 2, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 2, 2022
@openjdk
Copy link

openjdk bot commented Feb 2, 2022

@Michael-Mc-Mahon Pushed as commit 87ab099.

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

@Michael-Mc-Mahon Michael-Mc-Mahon deleted the winpipe2 branch February 2, 2022 15:12
*/
PipeImpl(SelectorProvider sp) throws IOException {
this(sp, true);
this(sp, true, false);

Choose a reason for hiding this comment

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

I’m pretty sure that this should actually be:

Suggested change
this(sp, true, false);
this(sp, /*AF_UNIX*/ false, /*buffering*/ true);

Copy link
Member

@dfuch dfuch Feb 23, 2022

Choose a reason for hiding this comment

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

Good catch. I will log a bug for that.

Copy link
Contributor

@AlanBateman AlanBateman Feb 23, 2022

Choose a reason for hiding this comment

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

Good catch. I will log a bug for that. In practice (and fortunately) it doesn't really matter since AFAICT this constructor is never called.

The SelectorProvider's openPipe method creates the pipe implementation with the 1-arg constructor. The Selector creates the pipe for the wakeup mechanism with the 3-arg constructor. This PR is about the latter rather than the former. It is true that the comment in constructor description doesn't match the implementation but I read the comment from ExE-Boss to be asking for the openPipe to return an implementation that uses TCP sockets. It wasn't the intention to change that, at least not until there Windows has a fix for the issue.

@mlbridge
Copy link

mlbridge bot commented Feb 23, 2022

Mailing list message from Daniel Fuchs on nio-dev:

On 23/02/2022 10:14, Daniel Fuchs wrote:

Good catch. I will log a bug for that. In practice (and fortunately) it doesn't really matter since AFAICT this constructor is never called.

Correction: the constructor is called once in SelectorProviderImpl.java.
Logged JDK-8282296

-- daniel

@Michael-Mc-Mahon
Copy link
Member Author

Sorry, my previous comment was wrong, which I deleted. Exe-Boss's comment is correct. The two booleans should be the other way round as the TCP based pipe socket should be buffered.

We can discuss further on the PR for the new issue, if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated nio nio-dev@openjdk.org
4 participants