Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
> 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 Patrick Concannon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - 8253119: Added message to null check in java/net/DatagramSocket - Merge remote-tracking branch 'origin/master' into JDK-8253119 - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from java/net/DatagramSocket - Merge remote-tracking branch 'origin/master' into JDK-8253119 - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation - Changes: - all: https://git.openjdk.java.net/jdk/pull/4574/files - new: https://git.openjdk.java.net/jdk/pull/4574/files/49125e7e..36bcee31 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4574&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4574&range=02-03 Stats: 7050 lines in 209 files changed: 1681 ins; 5063 del; 306 mod Patch: https://git.openjdk.java.net/jdk/pull/4574.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4574/head:pull/4574 PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Thu, 24 Jun 2021 15:06:49 GMT, Daniel Fuchs wrote: >>> 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 - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Fri, 25 Jun 2021 10:25:52 GMT, Patrick Concannon wrote: >> 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 > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8253119: Added message to null check in java/net/DatagramSocket > - Merge remote-tracking branch 'origin/master' into JDK-8253119 > - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from > java/net/DatagramSocket > - Merge remote-tracking branch 'origin/master' into JDK-8253119 > - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl > implementation Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Fri, 25 Jun 2021 10:25:52 GMT, Patrick Concannon wrote: >> 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 > > Patrick Concannon has updated the pull request with a new target base due to > a merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8253119: Added message to null check in java/net/DatagramSocket > - Merge remote-tracking branch 'origin/master' into JDK-8253119 > - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from > java/net/DatagramSocket > - Merge remote-tracking branch 'origin/master' into JDK-8253119 > - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl > implementation src/java.base/share/classes/java/net/DatagramSocket.java line 1398: > 1396: DatagramSocketImpl impl = > factory.createDatagramSocketImpl(); > 1397: Objects.requireNonNull(impl, > 1398: "Implementation returned by installed > DatagramSocketFactoryImpl is null"); I think you mean "DatagramSocketFactoryImpl" rather than "DatagramSocketImplFactory" here. - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Fri, 25 Jun 2021 10:56:15 GMT, Alan Bateman wrote: >> Patrick Concannon has updated the pull request with a new target base due to >> a merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - 8253119: Added message to null check in java/net/DatagramSocket >> - Merge remote-tracking branch 'origin/master' into JDK-8253119 >> - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from >> java/net/DatagramSocket >> - Merge remote-tracking branch 'origin/master' into JDK-8253119 >> - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl >> implementation > > src/java.base/share/classes/java/net/DatagramSocket.java line 1398: > >> 1396: DatagramSocketImpl impl = >> factory.createDatagramSocketImpl(); >> 1397: Objects.requireNonNull(impl, >> 1398: "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 - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]
On Fri, 25 Jun 2021 11:37:50 GMT, Daniel Fuchs wrote: >> src/java.base/share/classes/java/net/DatagramSocket.java line 1398: >> >>> 1396: DatagramSocketImpl impl = >>> factory.createDatagramSocketImpl(); >>> 1397: Objects.requireNonNull(impl, >>> 1398: "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 - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v5]
> 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 Patrick Concannon has updated the pull request incrementally with one additional commit since the last revision: 8253119: Fixed typo - Changes: - all: https://git.openjdk.java.net/jdk/pull/4574/files - new: https://git.openjdk.java.net/jdk/pull/4574/files/36bcee31..686b4369 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4574&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4574&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4574.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4574/head:pull/4574 PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v5]
On Fri, 25 Jun 2021 11:48:52 GMT, Patrick Concannon wrote: >> 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 > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8253119: Fixed typo Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v5]
On Fri, 25 Jun 2021 11:48:52 GMT, Patrick Concannon wrote: >> 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 > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8253119: Fixed typo Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4574
Re: RFR: 8268960: com/sun/net/httpserver/Headers.java: Ensure mutators normalize keys and disallow null for keys and values [v3]
On Thu, 24 Jun 2021 15:15:07 GMT, Daniel Fuchs wrote:
>> Julia Boes has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> confirm HttpExchange::sendResponseHeaders fails if key/value null
>
> src/jdk.httpserver/share/classes/com/sun/net/httpserver/Headers.java line 115:
>
>> 113: @Override
>> 114: public boolean containsKey(Object key) {
>> 115: Objects.requireNonNull(key);
>
> I haven't looked at the CSR yet - but maybe the fact that the get and
> contains* method can now throw NPE - in conformance to the Map interface
> specification for maps that do not allow null keys or null values, should be
> explicitly mentioned - in particular in the release note - as this will
> probably be the less expected behavior change.
Updated CSR to better reflect this behavioural change.
-
PR: https://git.openjdk.java.net/jdk/pull/4528
Re: RFR: 8268960: com/sun/net/httpserver/Headers.java: Ensure mutators normalize keys and disallow null for keys and values [v4]
> `com.sun.net.httpserver.Headers` normalizes its keys to adhere to the > following format: First character uppercase, all other characters lowercase, > for example `"foo" -> "Foo"`. This behaviour is not consistent across the > mutator methods of the class, in particular `putAll()` and `replaceAll()` do > not apply normalization. > > The suggested fix is to update the implementation of `putAll()` and to add an > implementation of of the java.util.Map default method `replaceAll()`. While > here, we can improve `equals()` by adding a type check and add a `toString()` > implementation. > > Additionally, the Headers class is updated to disallow null values for keys > and values. Julia Boes has updated the pull request incrementally with one additional commit since the last revision: revert equals, update toString, remove @throws NPE - Changes: - all: https://git.openjdk.java.net/jdk/pull/4528/files - new: https://git.openjdk.java.net/jdk/pull/4528/files/1e0ea047..5b009f11 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4528&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4528&range=02-03 Stats: 12 lines in 2 files changed: 1 ins; 7 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4528.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4528/head:pull/4528 PR: https://git.openjdk.java.net/jdk/pull/4528
[jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
More refactoring to limit the scope of `@SuppressWarnings` annotations. Sometimes I introduce new methods. Please feel free to suggest method names you like to use. - Commit messages: - 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K Changes: https://git.openjdk.java.net/jdk17/pull/152/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17&pr=152&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269409 Stats: 289 lines in 21 files changed: 161 ins; 64 del; 64 mod Patch: https://git.openjdk.java.net/jdk17/pull/152.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152 PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. Changes look good Max - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K
On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang wrote: > More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. LGTM. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/152
Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]
> More refactoring to limit the scope of `@SuppressWarnings` annotations. > > Sometimes I introduce new methods. Please feel free to suggest method names > you like to use. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: one more - Changes: - all: https://git.openjdk.java.net/jdk17/pull/152/files - new: https://git.openjdk.java.net/jdk17/pull/152/files/d8b384df..774eb9da Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=152&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=152&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk17/pull/152.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152 PR: https://git.openjdk.java.net/jdk17/pull/152
