8278326: Socket close is not thread safe and other cleanup

[AlanBateman]

java.net.Socket is not specified to be thread safe but it is required to support async close. If you create an unbound Socket and close it at around the time that another thread is binding, connecting, or anything else that creates the underlying socket then it can leak. The simplest thing to do is to synchronize all methods but the underlying SocketImpl implementation is thread safe, and all we really need is for Socket (and ServerSocket) to synchronize the creation of the underlying socket (SocketImpl.create) with close. As part of this change I've replaced the 6 flags with a bit mask. A new test is added to the Socket/asyncClose directory to test closing concurrently with another operation, the test will detect if the closed Socket is connected to a SocketImpl with an open socket.

Related is that ServerSocket.implAccept can be overridden to provide the Socket to accept. Its behavior is unspecified when called with a Socket that isn't newly created/unbound and there are number of silly scenarios that can arise. I've changed implAccept to coordinate with close so that accept doesn't return a closed Socket that is connected to an underlying socket. A new test is added to exercise these scenarios.

There are a couple of random cleanup/formatting nits in this patch.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8278326: Socket close is not thread safe and other cleanup

Reviewers

• Jaikiran Pai (@jaikiran - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11863/head:pull/11863
$ git checkout pull/11863

Update a local copy of the PR:
$ git checkout pull/11863
$ git pull https://git.openjdk.org/jdk pull/11863/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11863

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11863.diff

[bridgekeeper]

👋 Welcome back alanb! 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]

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

• net

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

• 02: Full - Incremental (71900694)
• 01: Full - Incremental (6184f476)
• 00: Full (3d62c906)

[jaikiran]

I've added some minor review comments, but overall the changes in this PR look good to me.

[jaikiran]

With the removal of synchronized from the Socket APIs, would this require a CSR?

[AlanBateman]

With the removal of synchronized from the Socket APIs, would this require a CSR?

"synchronized" isn't part of the signature and there isn't any behavior change there. The only change in behavior is in the silly (and unspecified) area of calling implAccept with a connected or closed socket.

[jaikiran]

The changes in this PR look good to me.
(Leaky.java line 231 has a typo in a comment, that I think you missed, but that's a trivial thing)

[openjdk]

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

8278326: Socket close is not thread safe and other cleanup

Reviewed-by: jpai

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

• f857f8a: 8299327: Allow super late barrier expansion of store barriers in C2
• 257f667: 8296789: -completion in jshell fails to expose synthetic bridge methods
• 030e88d: 8299829: In jshell, the output of "0".repeat(49999)+"2" ends with a '0'
• ef32fdb: 8284825: sun/java2d/DirectX/MultiPaintEventTest/MultiPaintEventTest.java failed with "RuntimeException: Processed unnecessary paint()."

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.

[AlanBateman]

(Leaky.java line 231 has a typo in a comment, that I think you missed, but that's a trivial thing)

Well spotted, I fixed the typo in another place so missed it in Leaky.

[dfuch]

This looks reasonable. It's a bit strange that shutdownInput() / shutdownOutput() are not protected by the same lock than close() but they were not before either.

[AlanBateman]

It's a bit strange that shutdownInput() / shutdownOutput() are not protected by the same lock than close() but they were not before either.

Thanks for going through this. The shutdownXXX methods require the Socket to be connected so they don't create the SocketImpl or the underlying socket. It's okay for these methods to race with each other, or close, as it will be handled by the SocketImpl (NioSocketImpl).

[AlanBateman]

/integrate

[openjdk]

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

• 036c808: 8298482: Implement ParallelGC NUMAStats for Linux
• 0ee8cac: 8299672: Enhance HeapDump JFR event
• d716ec5: 8299179: ArrayFill with store on backedge needs to reduce length by 1
• af8d3fb: 8264684: os::get_summary_cpu_info padded with spaces
• 78b1686: 8227257: javax/swing/JFileChooser/4847375/bug4847375.java fails with AssertionError
• 33f3bd8: Merge
• 945ef07: 8299862: OfAddress setter should disallow heap segments
• 636976a: 8299849: Revert JDK-8294461: wrong effectively final determination by javac
• 0abb87a: 8299227: host exif.org not found in link in doc comment
• de79162: 8299715: IR test: VectorGatherScatterTest.java fails with SVE randomly
• ... and 18 more: https://git.openjdk.org/jdk/compare/95b102c9b1e6a46771f4bea0ca2101c05476172d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@AlanBateman Pushed as commit 4b57334.

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