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

8285515: (dc) DatagramChannel.disconnect fails with "Invalid argument" on macOS 12.4 beta2 #8445

Closed
wants to merge 5 commits into from

Conversation

jaikiran
Copy link
Member

@jaikiran jaikiran commented Apr 28, 2022

Can I please get a review of this change with proposes to address the issue reported in https://bugs.openjdk.java.net/browse/JDK-8285515?

We have noticed that in the latest 12.4 Beta2 version of MacOSX, the implementation of DatagramChannelImpl.disconnect fails with an unexpected error.

Internally, the disconnect is implemented as a call to connect by passing it a "null" address as suggested in the documentation of man connect. In previous versions of MacOSX, this call used to return a EADDRNOTAVAIL errno and at the same time would correctly dissolve the connected association. The EADDRNOTAVAIL was then caught and intentionally ignored by the code in the JNI layer in DatagramChannelImpl.disconnect0.

In MacOSX 12.4 Beta 2 we are now seeing this connect call return EINVAL. This change in behaviour is noticed only for IPv4-mapped IPv6 addresses.

The man connect documentation on MacOSX states that even disconnectx can be used to disconnect datagram sockets. The commit here does that change to use disconnectx for MacOSX. Tests have been successfully run on some older Mac setups to make sure this change passes there as well.

Currently tier1, tier2 and tier3 run is in progress across various OS.

No additional tests have been added for this change since the many existing (failing) tests already cover this use case.


Progress

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

Issue

  • JDK-8285515: (dc) DatagramChannel.disconnect fails with "Invalid argument" on macOS 12.4 beta2

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8445

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 28, 2022

👋 Welcome back jpai! 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 Apr 28, 2022
@openjdk
Copy link

openjdk bot commented Apr 28, 2022

@jaikiran 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 Apr 28, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 28, 2022

Webrevs

@@ -50,20 +50,28 @@ Java_sun_nio_ch_DatagramChannelImpl_disconnect0(JNIEnv *env, jclass clazz,
jint fd = fdval(env, fdo);
int rv;

#if defined(__APPLE__)
// On MacOSX systems we use disconnectx
Copy link
Contributor

@AlanBateman AlanBateman Apr 28, 2022

Choose a reason for hiding this comment

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

Thanks for doing this. Do you mind changing this comment to "macOS" as I think that is how the OS is named these days.

Copy link
Member Author

@jaikiran jaikiran Apr 28, 2022

Choose a reason for hiding this comment

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

Done. I have pushed an update to this PR with this change.

@dfuch
Copy link
Member

dfuch commented Apr 28, 2022

That looks good to me. I'd suggest running some test repeat campaign on the CI to make sure this works as expected on all macOS versions that we support, and there's no regression on other OS.
I would also suggest tagging the tests that failed without this change with @bug 8285515 (even though they only failed when running on the 12.4 beta2)

@AlanBateman
Copy link
Contributor

AlanBateman commented Apr 28, 2022

I would also suggest tagging the tests that failed without this change with @bug 8285515 (even though they only failed when running on the 12.4 beta2)

Maybe okay to add it to one test for disconnect but please don't change every test that failed, it's just not helpful.

@jaikiran
Copy link
Member Author

jaikiran commented Apr 28, 2022

I would also suggest tagging the tests that failed without this change with @bug 8285515 (even though they only failed when running on the 12.4 beta2)

Maybe okay to add it to one test for disconnect but please don't change every test that failed, it's just not helpful.

Done. I updated the PR to add it to just one test case.

@jaikiran
Copy link
Member Author

jaikiran commented Apr 28, 2022

I'd suggest running some test repeat campaign on the CI to make sure this works as expected on all macOS versions that we support, and there's no regression on other OS.

tier1, tier2, tier3 tests that were in progress against various different OS, completed without any related failures.

I've additionally triggered a run with test repeat of 50, solely against macosx-x64 and macosx-aarch64. I'll let them run overnight and see how they go.

dfuch
dfuch approved these changes Apr 28, 2022
Copy link
Member

@dfuch dfuch left a comment

LGTM

@openjdk
Copy link

openjdk bot commented Apr 28, 2022

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

8285515: (dc) DatagramChannel.disconnect fails with "Invalid argument" on macOS 12.4 beta2

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

  • 36bf6fb: 8285728: Alpine Linux build fails with busybox tar
  • 091637c: 8285630: Fix a configure error in RISC-V cross build
  • ccf0e8b: 8285755: JDK-8285093 changed the default for --with-output-sync
  • d7514b0: 8285595: Assert frame anchor doesn't change in safepoints/handshakes
  • 5629c75: 8284848: C2: Compiler blackhole arguments should be treated as globally escaping
  • 85f8d14: 8283994: Make Xerces DatatypeException stackless
  • 4f2e4c7: 8178969: [TESTBUG] Wrong reporting of gc/g1/humongousObjects/TestHeapCounters test.

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 label Apr 28, 2022
@jaikiran
Copy link
Member Author

jaikiran commented Apr 29, 2022

The test runs completed without any related failures. Thank you Alan and Daniel for the reviews.

@jaikiran
Copy link
Member Author

jaikiran commented Apr 29, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Apr 29, 2022

Going to push as commit 269eae6.
Since your change was applied there have been 28 commits pushed to the master branch:

  • c4f7a85: 8285828: runtime/execstack/TestCheckJDK.java fails with zipped debug symbols
  • d3606a3: 8285390: PPC64: Handle integral division overflow during parsing
  • f42631e: 8285523: Improve test java/io/FileOutputStream/OpenNUL.java
  • b71e8c1: 8285711: riscv: RVC: Support disassembler show-bytes option
  • e406662: 8282711: Accelerate Math.signum function for AVX and AVX512 target.
  • 0a4a640: 8285301: C2: assert(!requires_atomic_access) failed: can't ensure atomicity
  • 40f19c0: 8264666: Change implementation of safeAdd/safeMult in the LCMSImageLayout class
  • 1e28fcb: 8155701: The compiler fails with an AssertionError: typeSig ERROR
  • 99388ef: 8283624: Create an automated regression test for RFE-4390885
  • 94b533a: 8285699: riscv: Provide information when hitting a HaltNode
  • ... and 18 more: https://git.openjdk.java.net/jdk/compare/47951655acacba515c0d69f5192257664f887dba...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Apr 29, 2022
@openjdk openjdk bot closed this Apr 29, 2022
@openjdk openjdk bot removed ready rfr labels Apr 29, 2022
@openjdk
Copy link

openjdk bot commented Apr 29, 2022

@jaikiran Pushed as commit 269eae6.

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

@jaikiran jaikiran deleted the 8285515 branch Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated nio
3 participants