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

[jaikiran]

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

• Daniel Fuchs (@dfuch - Reviewer)
• Alan Bateman (@AlanBateman - Reviewer)

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]

👋 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]

@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.

[mlbridge]

Webrevs

• 03: Full - Incremental (16de5522)
• 02: Full - Incremental (fd05ea60)
• 01: Full - Incremental (c25d1205)
• 00: Full (8cba69f0)

[AlanBateman]

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

[jaikiran]

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

[dfuch]

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]

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]

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]

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]

LGTM

[openjdk]

@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.

[jaikiran]

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

[jaikiran]

/integrate

[openjdk]

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]

@jaikiran Pushed as commit 269eae6.

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