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

8254692: (se) Clarify the behaviour of the non-abstract SelectorProvider::inheritedChannel #745

Closed
wants to merge 5 commits into from

Conversation

fguallini
Copy link
Contributor

@fguallini fguallini commented Oct 19, 2020

Please review this fix to provide spec clarity on the default implementation of java.nio.channels.spi.SelectorProvider::inheritedChannel() method which simply returns “null”, and a test for it.

Please review the corresponding CSR as well: https://bugs.openjdk.java.net/browse/JDK-8254848


Progress

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

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8254692: (se) Clarify the behaviour of the non-abstract SelectorProvider::inheritedChannel

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/745/head:pull/745
$ git checkout pull/745

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 19, 2020

👋 Welcome back fguallini! 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 Pull request is ready for review label Oct 19, 2020
@openjdk
Copy link

openjdk bot commented Oct 19, 2020

@fguallini 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 Oct 19, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 19, 2020

Webrevs

@openjdk
Copy link

openjdk bot commented Oct 19, 2020

@fguallini 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:

8254692: (se) Clarify the behaviour of the non-abstract SelectorProvider::inheritedChannel

Reviewed-by: chegar, bpb, 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 112 new commits pushed to the master branch:

  • c9269bf: 8255036: Shenandoah: Reset GC state for root verifier
  • 839f01d: 8242068: Signed JAR support for RSASSA-PSS and EdDSA
  • e559bd2: 8254889: name_and_sig_as_C_string usages in frame coding without ResourceMark
  • da97ab5: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer
  • 7e26404: 8255000: C2: Unify IGVN processing when loop opts are over
  • 27230fa: 8255026: C2: Miscellaneous cleanups in Compile and PhaseIdealLoop code
  • c107178: 8253964: [Graal] UnschedulableGraphTest#test01fails with expected:<4> but was:<3>
  • bd45191: 8255065: Zero: accessor_entry misses the IRIW case
  • 2a06335: 8254785: compiler/graalunit/HotspotTest.java failed with "missing Graal intrinsics for: java/lang/StringLatin1.indexOfChar([BIII)I"
  • 1b7ddeb: 8254976: Re-enable swing jtreg tests which were broken due to samevm mode
  • ... and 102 more: https://git.openjdk.java.net/jdk/compare/f3ce45f2a1ba0c91c601043125ca8691aa3deb36...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 (@ChrisHegarty, @bplb, @AlanBateman) 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 Pull request is ready to be integrated label Oct 19, 2020
bplb
bplb approved these changes Oct 19, 2020
Copy link
Member

@bplb bplb left a comment

Choose a reason for hiding this comment

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

Approved.

import static org.testng.Assert.assertThrows;

/*
* @test
* @bug 8254692
Copy link
Contributor

Choose a reason for hiding this comment

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

test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel would be a more appropriate location to test this.

I assume you modified etc/ProtocolFamilies.java you because it has a custom provider that doesn't override this method but this test has the summary "Test SocketChannel, ServerSocketChannel and DatagramChannel with various ProtocolFamily combinations" so not really the right place.

Copy link
Member

Choose a reason for hiding this comment

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

The test method being changed here assets other aspects of the default implementation of SelectorProvider - this is just another case. I guess is depends on ones view, whether these checks should be co-located based on functionality or based on category of behaviour. It might be best to create a new test/jdk/java/nio/channels/spi/SelectorProvider/DefaultImplementationTest.java to hold such, and move the other assertions there too. But the higher-order bit is that we have tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

ProtocolFamilies/testCustomProvider exercises the methods that take a protocol family, it's not really the right place to exercise the inheritedChannel method. Can we add a test to test/jdk/java/nio/channels/spi/SelectorProvider/inheritedChannel instead?

Copy link
Member

Choose a reason for hiding this comment

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

No disagreement about testCustomProvider not being the most appropriate place to test the default implementation of inheritedChannel. What I was suggesting was to move all the assertions related to the default implementation of SelectorProvider to a single new test located under test/jdk/java/nio/channels/spi/SelectorProvider/, rather than splitting the tests for the default implementation across several different tests and directory hierarchies.

Copy link
Contributor

Choose a reason for hiding this comment

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

A new test/jdk/java/nio/channels/spi/SelectorProvider that covers all the assertions would be great, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Alan and Chris for reviewing, I moved all the assertions related to the default implementation from ProtocolFamilies to a new test class as suggested and removed unused imports in ProtocolFamilies

@ChrisHegarty
Copy link
Member

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Oct 20, 2020
@openjdk
Copy link

openjdk bot commented Oct 20, 2020

@ChrisHegarty this pull request will not be integrated until the CSR request JDK-8254848 for issue JDK-8254692 has been approved.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Oct 20, 2020
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Oct 20, 2020
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 new test. SelectorProvider has 3 non-abstract methods and the coverage looks good. "TestDefaultImplementation" might be a better name for the test but otherwise looks good.

@fguallini
Copy link
Contributor Author

Thanks for the new test. SelectorProvider has 3 non-abstract methods and the coverage looks good. "TestDefaultImplementation" might be a better name for the test but otherwise looks good.

Sure, TestDefaultImplementation is a bit more accurate, therefore I renamed the test accordingly in the last commit.

@fguallini
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 21, 2020
@openjdk
Copy link

openjdk bot commented Oct 21, 2020

@fguallini
Your change (at version 4528ea4) is now ready to be sponsored by a Committer.

@AlanBateman
Copy link
Contributor

/sponsor

@openjdk openjdk bot closed this Oct 21, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 21, 2020
@openjdk
Copy link

openjdk bot commented Oct 21, 2020

@AlanBateman @fguallini Since your change was applied there have been 112 commits pushed to the master branch:

  • c9269bf: 8255036: Shenandoah: Reset GC state for root verifier
  • 839f01d: 8242068: Signed JAR support for RSASSA-PSS and EdDSA
  • e559bd2: 8254889: name_and_sig_as_C_string usages in frame coding without ResourceMark
  • da97ab5: 8253474: Javadoc clean up in HttpsExchange, HttpsParameters, and HttpsServer
  • 7e26404: 8255000: C2: Unify IGVN processing when loop opts are over
  • 27230fa: 8255026: C2: Miscellaneous cleanups in Compile and PhaseIdealLoop code
  • c107178: 8253964: [Graal] UnschedulableGraphTest#test01fails with expected:<4> but was:<3>
  • bd45191: 8255065: Zero: accessor_entry misses the IRIW case
  • 2a06335: 8254785: compiler/graalunit/HotspotTest.java failed with "missing Graal intrinsics for: java/lang/StringLatin1.indexOfChar([BIII)I"
  • 1b7ddeb: 8254976: Re-enable swing jtreg tests which were broken due to samevm mode
  • ... and 102 more: https://git.openjdk.java.net/jdk/compare/f3ce45f2a1ba0c91c601043125ca8691aa3deb36...master

Your commit was automatically rebased without conflicts.

Pushed as commit f813a28.

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

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