8336817: Several methods on DatagramSocket and MulticastSocket do not specify behaviour when already closed or connected #20543

jaikiran · 2024-08-12T08:18:17Z

Can I please get a review of this doc-only change which updates the javadoc on several methods of the java.net.DatagramSocket class to match its current implementation?

This addresses https://bugs.openjdk.org/browse/JDK-8336817.

I'll open a CSR once we settle on the proposed text.

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
Change requires CSR request JDK-8338397 to be approved

Issues

JDK-8336817: Several methods on DatagramSocket and MulticastSocket do not specify behaviour when already closed or connected (Bug - P4)
JDK-8338397: Several methods on DatagramSocket and MulticastSocket do not specify behaviour when already closed or connected (CSR)

Reviewers

Daniel Fuchs (@dfuch - Reviewer)
Alan Bateman (@AlanBateman - Reviewer) 🔄 Re-review required (review applies to d9bac1bf)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20543

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

Using diff file

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

Webrev

Link to Webrev Comment

…y connected

jaikiran · 2024-08-12T08:18:24Z

/csr needed

bridgekeeper · 2024-08-12T08:18:49Z

👋 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 · 2024-08-12T08:19:36Z

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

8336817: Several methods on DatagramSocket and MulticastSocket do not specify behaviour when already closed or connected

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

55851a3: 8281533: Odd "preview" label in link/linkplain
c7690c3: 8338190: TOC vertical offsets not updated when document size changes
6460b30: 8321140: Add comment to note difference in Metal's JButton margins
3ca359a: 8335771: Improve stability of java/nio/channels/DatagramChannel tests
2766b09: 8338452: (dc) DatagramChannelImpl.blockingReceive with timeout may block indefinitely if all datagrams blocked by SecurityManager
f0fe313: 8338564: Remove obsolete AbstractNamedEntry::equals method
6ff6b09: 8290501: Typo in javax.swing.BoundedRangeModel documentation
e07a5b6: 8338512: JFR: Revert changes to TestCodeSweeper
6d430f2: 8338314: JFR: Split JFRCheckpoint VM operation
f0374a0: 8337987: Relocate jfr and throw_exception stubs from StubGenerator to SharedRuntime
... and 75 more: https://git.openjdk.org/jdk/compare/03204600c596214895ef86581eba9722f76d39b3...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 · 2024-08-12T08:20:01Z

@jaikiran has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@jaikiran please create a CSR request for issue JDK-8336817 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2024-08-12T08:20:28Z

@jaikiran 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 · 2024-08-12T08:23:58Z

Webrevs

AlanBateman · 2024-08-13T08:17:23Z

src/java.base/share/classes/java/net/DatagramSocket.java

@@ -422,6 +422,10 @@ public void bind(SocketAddress addr) throws SocketException {
     * call to send or receive may throw a PortUnreachableException. Note,
     * there is no guarantee that the exception will be thrown.
     *
+     * <p> If this socket is already connected, then this method will attempt to
+     * connect to the passed address and if that attempt completes exceptionally,
+     * then this socket will continue to be connected to the previously connected address.


I don't know if there are other implementations DatagramSocketImpl in the eco system but the text could potentially invalidate them. I think it would be better to say that if connect fails then the state of the DatagramSocket is unknown, it may or may not be connected to the address that it was previously connected to.

Hello Alan,

I don't know if there are other implementations DatagramSocketImpl in the eco system but the text could potentially invalidate them.

I hadn't taken into account other implementations. I've now updated the PR to use the text that you suggested.

dfuch

This looks reasonable to me. Nice clarification.

AlanBateman · 2024-08-14T13:00:21Z

src/java.base/share/classes/java/net/DatagramSocket.java

@@ -422,6 +422,11 @@ public void bind(SocketAddress addr) throws SocketException {
     * call to send or receive may throw a PortUnreachableException. Note,
     * there is no guarantee that the exception will be thrown.
     *
+     * <p> If this socket is already connected, then this method will attempt to
+     * connect to the passed address and if the connect fails then the state of


A small suggestion is to say "given address" rather than "passed address" and I think break at this point so that "If this connect fails ..." is a new sentence.

Done. I've updated the PR with this change. I'll now create the CSR for review.

…erSocket.close()

jaikiran · 2024-08-14T13:36:27Z

When we did similar change in Socket/ServerSocket classes recently, Joe had suggested to add a note to the close() methods to say that several methods in those classes throw an exception when invoked on a closed instance. We had applied that suggestion to those classes. So I've now added a similar one-liner to the DatagramSocket.close() in this PR.

The CSR with these changes is now ready for review at https://bugs.openjdk.org/browse/JDK-8338397.

jaikiran · 2024-08-19T01:45:26Z

Joe rightly noted in the CSR about MulticastSocket. It too required clarifications similar to what we have done in the DatagramSocket. I've now updated this PR to include those doc-only changes.

dfuch

LGTM. One could argue that an I/O error encompasses the fact that the socket is closed, but what you have is fine, since "or the socket is closed" was added everywhere else.

jaikiran · 2024-08-20T01:16:41Z

The CSR has been approved. I took Alan's suggestion to update the title of the JBS issue and thus the CSR and this PR to more accurately state what this change now addresses.

I'll go ahead and integrate this later today.

jaikiran · 2024-08-20T12:27:28Z

/integrate

openjdk · 2024-08-20T12:28:56Z

Going to push as commit 686eb23.
Since your change was applied there have been 91 commits pushed to the master branch:

01d03e0: 8324209: Check implementation of Expect: 100-continue in the java.net.http.HttpClient
7933e45: 8338550: Do libubsan1 installation in test container only if requested
89ca5b6: 8338365: [PPC64, s390] Out-of-bounds array access in secondary_super_cache
b9d49dc: 8337981: ShenandoahHeap::is_in should check for alive regions
9775d57: 8338139: {ClassLoading,Memory}MXBean::isVerbose methods are inconsistent with their setVerbose methods
68d1f5c: 8338543: ClassBuilder withMethod builders should cache the method type symbol
55851a3: 8281533: Odd "preview" label in link/linkplain
c7690c3: 8338190: TOC vertical offsets not updated when document size changes
6460b30: 8321140: Add comment to note difference in Metal's JButton margins
3ca359a: 8335771: Improve stability of java/nio/channels/DatagramChannel tests
... and 81 more: https://git.openjdk.org/jdk/compare/03204600c596214895ef86581eba9722f76d39b3...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-20T12:29:01Z

@jaikiran Pushed as commit 686eb23.

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

8336817: DatagramSocket.connect does not specify behavior when alread…

ee4d332

…y connected

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Aug 12, 2024

openjdk bot added the net net-dev@openjdk.org label Aug 12, 2024

AlanBateman reviewed Aug 13, 2024

View reviewed changes

Alan's suggestion

06ca4aa

dfuch approved these changes Aug 14, 2024

View reviewed changes

AlanBateman reviewed Aug 14, 2024

View reviewed changes

jaikiran added 2 commits August 14, 2024 18:38

Minor adjustment to the doc

27df272

add a note to close() method just like we did for Socket.close()/Serv…

d9bac1b

…erSocket.close()

AlanBateman approved these changes Aug 14, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Aug 16, 2024

address javadoc on methods of MulticastSocket too

bfdd777

openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels Aug 19, 2024

dfuch approved these changes Aug 19, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration ready Pull request is ready to be integrated labels Aug 19, 2024

jaikiran changed the title ~~8336817: DatagramSocket.connect does not specify behavior when already connected~~ 8336817: Several methods on DatagramSocket and MulticastSocket do not specify behaviour when already closed or connected Aug 20, 2024

openjdk bot added the ready Pull request is ready to be integrated label Aug 20, 2024

openjdk bot added the integrated Pull request has been integrated label Aug 20, 2024

openjdk bot closed this Aug 20, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 20, 2024

jaikiran deleted the 8336817 branch August 20, 2024 12:29

8336817: Several methods on DatagramSocket and MulticastSocket do not specify behaviour when already closed or connected #20543

8336817: Several methods on DatagramSocket and MulticastSocket do not specify behaviour when already closed or connected #20543

Uh oh!

Conversation

jaikiran commented Aug 12, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

jaikiran commented Aug 12, 2024

Uh oh!

bridgekeeper bot commented Aug 12, 2024

Uh oh!

openjdk bot commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 12, 2024

Uh oh!

openjdk bot commented Aug 12, 2024

Uh oh!

mlbridge bot commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

AlanBateman Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

jaikiran Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

jaikiran Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Aug 14, 2024

Uh oh!

jaikiran commented Aug 19, 2024

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

jaikiran commented Aug 20, 2024

Uh oh!

jaikiran commented Aug 20, 2024

Uh oh!

openjdk bot commented Aug 20, 2024

Uh oh!

openjdk bot commented Aug 20, 2024

Uh oh!

Uh oh!

jaikiran commented Aug 12, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 12, 2024 •

edited

Loading

mlbridge bot commented Aug 12, 2024 •

edited

Loading