8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException #25031

jaikiran · 2025-05-05T10:12:11Z

Can I please get a review of this change which proposes to respecify the 2 java.net.Socket constructors that allow construction of UDP sockets? This addresses https://bugs.openjdk.org/browse/JDK-8356154.

As noted in that JBS issue, in Java 23 we deprecated for removal the 2 Socket constructors that allowed UDP socket creation. The plan continues to be to remove those constructors. Before removing those, in order to allow for applications to notice this deprecation, these constructors are now being respecified to throw an IllegalArgumentException when the stream parameter is false.

I will create a CSR once we settle on these changes.

tier1 through tier8 tests have been run with this change and no related failures have been noticed.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change requires CSR request JDK-8356225 to be approved
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException (Enhancement - P4)
JDK-8356225: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException (CSR)

Reviewers

Alan Bateman (@AlanBateman - Reviewer) Review applies to ac71c8a6
Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25031

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…DP sockets to throw IllegalArgumentException

jaikiran · 2025-05-05T10:13:01Z

/csr

bridgekeeper · 2025-05-05T10:13:10Z

👋 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 · 2025-05-05T10:13:56Z

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

8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException

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

c8a30c2: 8347004: vmTestbase/metaspace/shrink_grow/ShrinkGrowTest/ShrinkGrowTest.java fails with CDS disabled
90f0f1b: 8354443: [Graal] crash after deopt in TestG1BarrierGeneration.java
d7e648b: 8355775: Improve symbolic sharing in dynamic constant pool entries
... and 61 more: https://git.openjdk.org/jdk/compare/f1e06c3c050b3440555d63b6de80fe9f60b35dfa...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 · 2025-05-05T10:14:36Z

@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-8356154 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2025-05-05T10:15:01Z

@jaikiran The following labels will be automatically applied to this pull request:

net
nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-05-05T10:18:07Z

Webrevs

jaikiran · 2025-05-05T10:19:55Z

src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java

@@ -451,19 +449,20 @@ private void write(byte[] b, int off, int len) throws IOException {
     */
    @Override
    protected void create(boolean stream) throws IOException {
+        if (!stream) {
+            throw new IllegalArgumentException("datagram socket creation not supported");


My initial thought was to just assert the stream value here. Then I noticed the test/jdk/java/net/SocketImpl/BadUsages.java test (updated as part of this PR) and thought that it might be better to do an actual check here and have it throw IllegalArgumentException, to allow for this behaviour to be verified.

I however don't have a strong opinion about this. So if assert is enough, then I'll switch this to an assert and then remove the updated test method in the BasUsages.java.

This method can only throw IOException so I think it will need to throw IOException. It should happen of course, at least not unless we have a bug in the Socket code. Can you change the exception message to start with "Datagram socket ..." so it's consistent with the other exception messages.

Done - I've updated the PR to follow these suggestions. The BadUsages test passes with this change.

eirbjo · 2025-05-05T12:11:16Z

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

@@ -82,8 +82,12 @@ public SocketImpl() { }
     *
     * @apiNote
     * The {@link Socket} constructors to create a datagram socket
-     * are deprecated for removal. This method will be re-specified
-     * in a future release to not support creating datagram sockets.
+     * are deprecated for removal and have been respecified to throw


This seems to talk about past, current and future behavior.

I thought we try to keep specifications focused on current behavior, with the exception of deprecation warnings.

Would it be possible to reword this without mentioning the past, avoiding the “have been respecified” part?

Interested users can always use release notes to observe history..?

Hello Eirik,

This seems to talk about past, current and future behavior.
...
Would it be possible to reword this without mentioning the past, avoiding the “have been respecified” part?

That's a good point. I've now updated the PR to reword this. Hopefully that's better.

AlanBateman · 2025-05-05T13:52:35Z

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

-     * creates a datagram socket.
+     * stream socket. Only stream socket creation is allowed. If the stream
+     * argument is {@code false}, then this constructor throws
+     * {@code IllegalArgumentException}.


I would be tempted to drop this paragraph and just change the description of the @param stream to say "must be true".

I re-read this paragraph again today and I think it would be better to remove it completely from both constructors.

In my previous round I misread that the review comment was on SocketImpl class, so I never did apply the review changes on these Socket constructor.

I've now updated the PR to drop this paragraph from both these constructors. I'll update the CSR if this and the rest changes look good.

AlanBateman · 2025-05-05T14:00:39Z

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

+     * this method.
+     * <p>
+     * This method will be re-specified in a future release to not
+     * support creating datagram sockets.


I think this could do with another round of rewording. I think I would say that the "stream" parameter provided a way in early JDK releases to create a Socket that used a datagram socket. It's no longer possible to do this and therefore this method will only be called by Socket with stream == false.

I pushed a change to take these suggestions. I did some minor changes to the wording. Do you recommend any further changes to that text?

…iew suggestions

jaikiran · 2025-05-06T04:45:11Z

I've created a CSR https://bugs.openjdk.org/browse/JDK-8356225 and is ready for review.

AlanBateman · 2025-05-06T06:32:33Z

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

     * <p>
     * If the application has specified a {@linkplain SocketImplFactory client
     * socket implementation factory}, that factory's
     * {@linkplain SocketImplFactory#createSocketImpl() createSocketImpl}
     * method is called to create the actual socket implementation. Otherwise
     * a system-default socket implementation is created.
-     * <p>
-     * If a UDP socket is used, TCP/IP related socket options will not apply.
     *
     * @param      host     the host name, or {@code null} for the loopback address.
     * @param      port     the port number.
     * @param      stream   a {@code boolean} indicating whether this is


I think the parameter description has to be changed as it no longer indicates if the socket is a stream or datatgram socket. I think you can replace with something simple, like must be true, false is not allowed.

AlanBateman · 2025-05-06T08:03:27Z

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

-     * in a future release to not support creating datagram sockets.
+     * The {@code stream} parameter provided a way in early JDK releases
+     * to create a {@link Socket} that used a datagram socket.
+     * It is no longer possible to do that and therefore this method will


"It is no longer possible to do that ...", can you try changing this to "The Socket API no longer provides a way to do this so the create method will ways to be called with a stream value of true". I think it might be a bit easier to read.

AlanBateman · 2025-05-06T08:05:02Z

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

-     * If the stream argument is {@code true}, this creates a
-     * stream socket. If the stream argument is {@code false}, it
-     * creates a datagram socket.
-     * <p>


One final though on the Socket changes is that maybe we should include an API note to say that the stream parameter provided a way in early JDK releases to create a Socket that used a datagram socket, this feature no longer exists.

AlanBateman

Spec + implementation changes looks fine, I have not spent time on the test changes.

Michael-Mc-Mahon · 2025-05-06T13:07:13Z

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

+     * to create a {@code Socket} that used a datagram socket, this feature
+     * no longer exists.


Suggested change

* to create a {@code Socket} that used a datagram socket, this feature

* no longer exists.

* to create a {@code Socket} that used a datagram socket. This feature

* no longer exists.

Michael-Mc-Mahon · 2025-05-06T13:07:34Z

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

+     * to create a {@code Socket} that used a datagram socket, this feature
+     * no longer exists.


Suggested change

* to create a {@code Socket} that used a datagram socket, this feature

* no longer exists.

* to create a {@code Socket} that used a datagram socket. This feature

* no longer exists.

Michael-Mc-Mahon · 2025-05-06T13:08:46Z

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

@@ -454,6 +450,10 @@ private Socket(SocketAddress address, SocketAddress localAddr, boolean stream)
        throws IOException
    {
        Objects.requireNonNull(address);
+        if (!stream) {
+            throw new IllegalArgumentException(
+                    "Socket constructor does not support creation of datagram socket");


Suggested change

"Socket constructor does not support creation of datagram socket");

"Socket constructor does not support creation of datagram sockets");

Thank you Michael, I updated the PR with all 3 suggestions. The CSR text too has been updated with this minor update.

dfuch · 2025-05-06T15:22:02Z

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

+     *
+     * @apiNote
+     * The {@code stream} parameter provided a way in early JDK releases
+     * to create a {@code Socket} that used a datagram socket. This feature
+     * no longer exists.


Should we also re-iterate here that this constructor is deprecated? It kind of feels like this information should be in @deprecated instead, or that it should say that IAE is being thrown...

Hello Daniel, do you mean something like this:

- * @apiNote - * The {@code stream} parameter provided a way in early JDK releases - * to create a {@code Socket} that used a datagram socket. This feature - * no longer exists. - * * @param host the IP address. * @param port the port number. * @param stream must be true, false is not allowed. @@ -429,7 +424,9 @@ public Socket(String host, int port, boolean stream) throws IOException { * or if the port parameter is outside the specified range of valid * port values, which is between 0 and 65535, inclusive. * @throws NullPointerException if {@code host} is null. - * @deprecated Use {@link DatagramSocket} instead for UDP transport. + * @deprecated The {@code stream} parameter provided a way in early JDK releases + * to create a {@code Socket} that used a datagram socket. This feature + * no longer exists. Use {@link DatagramSocket} instead for UDP transport.

Having that text in @deprecated does convey the reason for deprecation and does show up prominently in the rendered javadoc:

So if you and others think we should remove the apiNote and move the text to deprecated, then I'll update the PR and the CSR.

Okay with me but for SocketImpl then I think it has to be an apiNote as the create method is not deprecated.

BTW: "Use DatagramSocket instead for UDP transport" switches the terminology. The first sentence uses "datagram socket" so I best to use that in the second sentence to avoid "UDP" and "transport".

Okay with me but for SocketImpl then I think it has to be an apiNote as the create method is not deprecated.

Makes sense.

BTW: "Use DatagramSocket instead for UDP transport" switches the terminology. The first sentence uses "datagram socket" so I best to use that in the second sentence to avoid "UDP" and "transport".

I agree, I'll use "datagram socket".

Thanks Jaikiran. Yes - it feels weird to add an @apiNote to a method or constructor that has long been deprecated: if it's deprecated you're not supposed to use it. I couldn't help feeling that using an @apiNote there conveyed the wrong message. I am completely OK with the @apiNote on the create method though - since that one isn't deprecated.

I've updated the PR to move the text into @deprecated section for these 2 constructors. I'll update the CSR once this new text is approved.

dfuch

Thanks! I prefer the new version.

jaikiran · 2025-05-08T03:56:23Z

Thank you all for the reviews, the CSR has been approved with this latest text. I'll go ahead with the integration.

jaikiran · 2025-05-08T03:56:34Z

/integrate

openjdk · 2025-05-08T03:56:45Z

Going to push as commit 52a5583.
Since your change was applied there have been 73 commits pushed to the master branch:

3e258cb: 8356407: Part of class verification is skipped in AOT training run
7642556: 8343110: Add getChars(int, int, char[], int) to CharSequence and CharBuffer
493ac93: 8244533: Configure should abort on missing short names in Windows
... and 70 more: https://git.openjdk.org/jdk/compare/f1e06c3c050b3440555d63b6de80fe9f60b35dfa...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-08T03:56:52Z

@jaikiran Pushed as commit 52a5583.

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

jaikiran added 2 commits May 5, 2025 15:15

8356154: Respecify java.net.Socket constructors that allow creating U…

5ed7b82

…DP sockets to throw IllegalArgumentException

update apiNote on SocketImpl

47dca9c

openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels May 5, 2025

openjdk bot added nio nio-dev@openjdk.org net net-dev@openjdk.org labels May 5, 2025

jaikiran commented May 5, 2025

View reviewed changes

review suggestion - throw IOException

5e12308

eirbjo reviewed May 5, 2025

View reviewed changes

reword SocketImpl.create(...) API doc

ad880d5

AlanBateman reviewed May 5, 2025

View reviewed changes

additional changes to SocketImpl.create(...) API javadoc based on rev…

8e329ff

…iew suggestions

AlanBateman reviewed May 6, 2025

View reviewed changes

Alan's review suggestion - simplify Socket constructor API documentation

9d41bbe

AlanBateman reviewed May 6, 2025

View reviewed changes

jaikiran added 2 commits May 6, 2025 14:31

additional updates to apiNotes

19cba4d

remove unintentional file additions

ac71c8a

AlanBateman approved these changes May 6, 2025

View reviewed changes

Michael-Mc-Mahon reviewed May 6, 2025

View reviewed changes

Michael's review suggestions

4cbe517

dfuch reviewed May 6, 2025

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label May 6, 2025

move apiNote text to deprecated text

77a332b

dfuch approved these changes May 7, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration labels May 7, 2025

openjdk bot added the integrated Pull request has been integrated label May 8, 2025

openjdk bot closed this May 8, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 8, 2025

		* to create a {@code Socket} that used a datagram socket, this feature
		* no longer exists.

	"Socket constructor does not support creation of datagram socket");
	"Socket constructor does not support creation of datagram sockets");

8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException #25031

8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException #25031

Uh oh!

Conversation

jaikiran commented May 5, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

jaikiran commented May 5, 2025

Uh oh!

bridgekeeper bot commented May 5, 2025

Uh oh!

openjdk bot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 5, 2025

Uh oh!

openjdk bot commented May 5, 2025

Uh oh!

mlbridge bot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran commented May 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jaikiran commented May 5, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 5, 2025 •

edited

Loading

mlbridge bot commented May 5, 2025 •

edited

Loading

dfuch May 6, 2025 •

edited

Loading