On Mon, 5 May 2025 10:19:55 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> 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. > > src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java line 453: > >> 451: protected void create(boolean stream) throws IOException { >> 452: if (!stream) { >> 453: 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25031#discussion_r2073230057
