8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation #4574

pconcannon · 2021-06-23T12:06:41Z

Hi,

Could someone please review my changes for the removal of the legacy PlainSocketImpl and PlainDatagramSocketImpl implementations?

In JDK 13, JEP 353 provided a drop in replacement for the legacy PlainSocketImpl implementation. Since JDK 13, the PlainSocketImpl implementation was no longer used but included a mitigation mechanism to reduce compatibility risks in the form of a JDK-specific property jdk.net.usePlainSocketImpl allowing to switch back to the old implementation.
Similarly, in JDK 15, JEP 373 provided a new implementation for DatagramSocket and MulticastSocket, with a JDK-specific property jdk.net.usePlainDatagramSocketImpl also allowing the user to switch back to the old implementation in case of compatibility issue.

As these implementations (and the mechanisms they use to enable them to mitigate compatibility issues) have been deemed no longer necessary, they now represent a maintenance burden. This patch looks at removing them from the JDK.

Kind regards,
Patrick

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation

Reviewers

Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 686b436
Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to 686b436
Chris Hegarty (@ChrisHegarty - Reviewer) ⚠️ Review applies to 49125e7

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4574/head:pull/4574
$ git checkout pull/4574

Update a local copy of the PR:
$ git checkout pull/4574
$ git pull https://git.openjdk.java.net/jdk pull/4574/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4574

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4574.diff

…l implementation

bridgekeeper · 2021-06-23T12:07:54Z

👋 Welcome back pconcannon! 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.

pconcannon · 2021-06-23T12:08:03Z

/csr needed

openjdk · 2021-06-23T12:08:58Z

@pconcannon has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@pconcannon please create a CSR request for issue JDK-8253119. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2021-06-23T12:10:35Z

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

net
nio
serviceability

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 · 2021-06-23T12:15:11Z

Webrevs

AlanBateman

Good cleanup, just a few nits in the changes to the NioSocketImpl impl, I didn't spot any other issues.

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

AlanBateman

I assume DatagramSocket L1043-L1049 (declaration/init of USE_PLAINDATAGRAMSOCKET) can be removed too.

AlanBateman · 2021-06-23T13:05:24Z

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

-                    impl = DefaultDatagramSocketImplFactory.createDatagramSocketImpl(multicast);
-                }
+                DatagramSocketImpl impl = factory.createDatagramSocketImpl();
+                Objects.requireNonNull(impl);


I assume this Objects.requireNonNull is not needed or maybe you want to detect a buggy factory rather than in the the constructor?

Yes - the constructor would throw NPE - but there's no need to call this constructor if impl is null here - it's better to fail early.

This is probably a case where the spec needs say that an error (maybe an unspecified error) is thrown if the custom factory's createDatagramSocketImpl returns null. We can create a separate issue for that as it's probably existed forever.

@AlanBateman

protected DatagramSocket(DatagramSocketImpl impl) throws NPE if impl is null.
(this is covered by the blanket statement for NPE)

Do we really need to specify anything else since the global setDatagramSocketImplFactory method has been deprecated? I mean - though we still support it at the moment, using a DatagramSocketImplFactory is not recommended, and hopefully only old legacy code would be using that.

Creating an issue to update DatagramSocketImplFactory.createDatagramSocketImpl to say that returning null leads to undefined behavior or DatagramSocket constructors throwing an error is fine, I wasn't suggesting we have to fix it here.

I've created an issue to track this: https://bugs.openjdk.java.net/browse/JDK-8269288

I've created an issue to track this: https://bugs.openjdk.java.net/browse/JDK-8269288

Thanks. So are you keeping the Objects.requireNonNull here? If so then it should probably be the 2-arg version so that the message is clear that the custom DatagramSocketFactory returned null, otherwise it will look like a DS bug.

@AlanBateman Ah - good idea - something like "implementation returned by installed DatagramSocketFactoryImpl is null"?
Maybe we could add the name of the concrete DatagramSocketFactoryImpl class too?

OK. I've added a message to the null check now. See 36bcee3

src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c

…java/net/DatagramSocket

pconcannon · 2021-06-24T11:40:06Z

I assume DatagramSocket L1043-L1049 (declaration/init of USE_PLAINDATAGRAMSOCKET) can be removed too.

USE_PLAINDATAGRAMSOCKET removed as requested. See 49125e7

AlanBateman · 2021-06-25T10:56:15Z

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

-                }
+                DatagramSocketImpl impl = factory.createDatagramSocketImpl();
+                Objects.requireNonNull(impl,
+                        "Implementation returned by installed DatagramSocketFactoryImpl is null");


I think you mean "DatagramSocketImplFactory" instead of "DatagramSocketFactoryImpl" here.

Yes - good catch! I've been bitten by this before too :-)
https://docs.oracle.com/en/java/javase/16/docs/api/java.base/java/net/DatagramSocketImplFactory.html

Well spotted. Thanks Alan. Change made in commit 686b436

openjdk · 2021-07-02T18:14:32Z

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

8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation

Reviewed-by: alanb, dfuchs, chegar

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

pconcannon · 2021-07-06T13:44:35Z

/integrate

openjdk · 2021-07-06T13:46:17Z

Going to push as commit 326b2e1.
Since your change was applied there have been 3 commits pushed to the master branch:

f485171: 8269692: sun.net.httpserver.ServerImpl::createContext should throw IAE
16aa8cb: 8269697: JNI_GetPrimitiveArrayCritical() should not accept object array
e47803a: 8266310: deadlock between System.loadLibrary and JNI FindClass loading another class

Your commit was automatically rebased without conflicts.

openjdk · 2021-07-06T13:46:35Z

@pconcannon Pushed as commit 326b2e1.

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

8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImp…

1aa471c

…l implementation

openjdk bot added the rfr Pull request is ready for review label Jun 23, 2021

openjdk bot added the csr Pull request needs approved CSR before integration label Jun 23, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org nio nio-dev@openjdk.org net net-dev@openjdk.org labels Jun 23, 2021

AlanBateman reviewed Jun 23, 2021

View reviewed changes

src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java Outdated Show resolved Hide resolved

src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java Show resolved Hide resolved

AlanBateman reviewed Jun 23, 2021

View reviewed changes

pconcannon added 2 commits June 24, 2021 12:24

Merge remote-tracking branch 'origin/master' into JDK-8253119

4f04b85

8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from …

49125e7

…java/net/DatagramSocket

dfuch approved these changes Jun 24, 2021

View reviewed changes

ChrisHegarty approved these changes Jun 24, 2021

View reviewed changes

pconcannon added 2 commits June 25, 2021 11:19

Merge remote-tracking branch 'origin/master' into JDK-8253119

4ae638a

8253119: Added message to null check in java/net/DatagramSocket

36bcee3

dfuch approved these changes Jun 25, 2021

View reviewed changes

AlanBateman reviewed Jun 25, 2021

View reviewed changes

8253119: Fixed typo

686b436

AlanBateman approved these changes Jun 25, 2021

View reviewed changes

dfuch approved these changes Jun 25, 2021

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Jul 2, 2021

openjdk bot added the ready Pull request is ready to be integrated label Jul 2, 2021

Merge remote-tracking branch 'origin/master' into JDK-8253119

538b145

openjdk bot closed this Jul 6, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 6, 2021

pconcannon mentioned this pull request Jul 6, 2021

8269917: Insert missing commas in copyrights in java.net #4690

Closed

3 tasks

8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation #4574

8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation #4574

pconcannon commented Jun 23, 2021 •

edited by openjdk bot

bridgekeeper bot commented Jun 23, 2021

pconcannon commented Jun 23, 2021

openjdk bot commented Jun 23, 2021

openjdk bot commented Jun 23, 2021

mlbridge bot commented Jun 23, 2021 •

edited

AlanBateman left a comment

AlanBateman left a comment

AlanBateman Jun 23, 2021

dfuch Jun 23, 2021

AlanBateman Jun 23, 2021

dfuch Jun 23, 2021 •

edited

AlanBateman Jun 23, 2021

pconcannon Jun 24, 2021

AlanBateman Jun 24, 2021

dfuch Jun 24, 2021

pconcannon Jun 25, 2021

pconcannon commented Jun 24, 2021

AlanBateman Jun 25, 2021 •

edited

dfuch Jun 25, 2021

pconcannon Jun 25, 2021

openjdk bot commented Jul 2, 2021 •

edited

pconcannon commented Jul 6, 2021

openjdk bot commented Jul 6, 2021

openjdk bot commented Jul 6, 2021

8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation #4574

8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation #4574

Conversation

pconcannon commented Jun 23, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jun 23, 2021

pconcannon commented Jun 23, 2021

openjdk bot commented Jun 23, 2021

openjdk bot commented Jun 23, 2021

mlbridge bot commented Jun 23, 2021 • edited

Webrevs

AlanBateman left a comment

Choose a reason for hiding this comment

AlanBateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfuch Jun 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pconcannon commented Jun 24, 2021

AlanBateman Jun 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jul 2, 2021 • edited

pconcannon commented Jul 6, 2021

openjdk bot commented Jul 6, 2021

openjdk bot commented Jul 6, 2021

pconcannon commented Jun 23, 2021 •

edited by openjdk bot

mlbridge bot commented Jun 23, 2021 •

edited

dfuch Jun 23, 2021 •

edited

AlanBateman Jun 25, 2021 •

edited

openjdk bot commented Jul 2, 2021 •

edited