Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v4]
> This sketch shows how "Stable Updaters" can be used to create stable > computations of `@Stable` fields. Only one updater is needed per class, > similar to `AtomicIntegerFieldUpdater`. Per Minborg has updated the pull request incrementally with two additional commits since the last revision: - Reformat - Revert changes in public classes - Changes: - all: https://git.openjdk.org/jdk/pull/25040/files - new: https://git.openjdk.org/jdk/pull/25040/files/83ce9ac1..4a42b271 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=02-03 Stats: 62 lines in 2 files changed: 17 ins; 37 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/25040.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25040/head:pull/25040 PR: https://git.openjdk.org/jdk/pull/25040
RFR: 7046003: Default value of Authenticator.getRequestingURL() is not specified
Clarificatiion of spec to outline that Authenticator.getRequestingURL() returns null in the event that the corresponding request does not specify a URL. - Commit messages: - authenticator getRequestingURL spec update - update inetaddress javadoc Changes: https://git.openjdk.org/jdk/pull/25097/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25097&range=00 Issue: https://bugs.openjdk.org/browse/JDK-7046003 Stats: 6 lines in 2 files changed: 3 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/25097.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25097/head:pull/25097 PR: https://git.openjdk.org/jdk/pull/25097
Re: RFR: 8355578: [java.net] Use @requires tag instead of exiting based on "os.name" property value [v2]
On Fri, 2 May 2025 09:15:24 GMT, Volkan Yazici wrote: >> Replaces `os.name` checks in tests with `@requires`. This prevents these >> tests from being run, and superficial results being generated, on >> unnecessary platforms. >> >> Note that `os.name` checks are replaced with `os.family` instead. > > Volkan Yazici has updated the pull request incrementally with one additional > commit since the last revision: > > Document `@requires` rationale with `@comment` > > Co-authored-by: Daniel Fuchs <67001856+df...@users.noreply.github.com> test/jdk/java/net/MulticastSocket/PromiscuousIPv6.java line 27: > 25: * @test > 26: * @bug 8215294 > 27: * @requires os.family == "linux" & !(os.version ~= "3\\.10\\.0.*") Would that match 13.10.01? - PR Review Comment: https://git.openjdk.org/jdk/pull/24997#discussion_r2077418883
Re: RFR: 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException [v8]
On Wed, 7 May 2025 10:37:47 GMT, Jaikiran Pai wrote: >> 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/25031#discussion_r2077398768
Re: RFR: 8355578: [java.net] Use @requires tag instead of exiting based on "os.name" property value [v2]
On Fri, 2 May 2025 09:15:24 GMT, Volkan Yazici wrote: >> Replaces `os.name` checks in tests with `@requires`. This prevents these >> tests from being run, and superficial results being generated, on >> unnecessary platforms. >> >> Note that `os.name` checks are replaced with `os.family` instead. > > Volkan Yazici has updated the pull request incrementally with one additional > commit since the last revision: > > Document `@requires` rationale with `@comment` > > Co-authored-by: Daniel Fuchs <67001856+df...@users.noreply.github.com> Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/24997#pullrequestreview-2821461900
RFR: 8356395: Spec needs to be clarified for InterfaceAddress.getBroadcast() method
Spec currently suggests that only IPv6 addresses can return null for InterfaceAddress.getBroadcast(). Clarifying spec to state that certain IPv4 address such as the loopback address do not support broadcasting and can therefore also return null. - Commit messages: - update inetaddress javadoc Changes: https://git.openjdk.org/jdk/pull/25095/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25095&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8356395 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/25095.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25095/head:pull/25095 PR: https://git.openjdk.org/jdk/pull/25095
Re: RFR: 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException [v9]
On Wed, 7 May 2025 11:34:08 GMT, Jaikiran Pai 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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > move apiNote text to deprecated text Thank you all for the reviews, the CSR has been approved with this latest text. I'll go ahead with the integration. - PR Comment: https://git.openjdk.org/jdk/pull/25031#issuecomment-2861663091
Integrated: 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException
On Mon, 5 May 2025 10:12:11 GMT, Jaikiran Pai 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. This pull request has now been integrated. Changeset: 52a5583d Author:Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/52a5583d691388f833c3aeb56ce92cbfb5d61274 Stats: 172 lines in 9 files changed: 37 ins; 72 del; 63 mod 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException Reviewed-by: dfuchs, alanb - PR: https://git.openjdk.org/jdk/pull/25031
Re: RFR: 8249824: s/n/w/p/https/HttpsURLConnection/CloseKeepAliveCached.java uses @ignore w/o bugid [v3]
On Thu, 6 Feb 2025 15:54:47 GMT, Mikhail Yankelevich wrote: >> * fully automated the test >> * removed the race condition >> * client on a thread and server on a thread options are now run together >> automatically > > Mikhail Yankelevich has updated the pull request incrementally with one > additional commit since the last revision: > > minor comment changes I understand this is a old test and it is not in scope of this bug to update existing code. But there are few issues in this test that should be addressed, for instance, use of deprecated URL, old style threads, old code styles, typos etc. Can you please check the complete test code as if you are to rewrite? - PR Comment: https://git.openjdk.org/jdk/pull/23469#issuecomment-2861858937
Re: RFR: 8341346: Add support for exporting TLS Keying Material [v7]
On Wed, 7 May 2025 16:28:27 GMT, Weijun Wang wrote: >> Bradford Wetmore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Updated to use the upcoming KDF (still in preview) + bits of JDK-8353578 >> for compilation) > > src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 1694: > >> 1692: >> 1693: // ...now the final expand. >> 1694: SecretKey key = hkdf.deriveKey(label, > > PKCS #11 is picky about key algorithm names and I'm not sure if `label` is > always accepted. The KDF API has the algorithm in the method arguments so > it's left to user to specify one. I'm not sure how the export keying material > will be used. If it is used in encryption, the algorithm may need to be > something like "AES". IIUC, the exported keying material can be used for any purpose or algorithm, so we really can't make an good educated guess what it might be. They could be Keys (Ciphers), byte array/value challenges, or even just data that will be signed. This is just doing a quick read of some of the IANA definitions which link to some of the known use cases: https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#exporter-labels Thus the type needs to be something generic. The label sounded good initially, but there is no specific prohibition against non-null/empty label in the TLS Exporters, but KDF prohibits null/empty labels. Maybe a label like "ExportKeyingMaterial"? - PR Review Comment: https://git.openjdk.org/jdk/pull/24976#discussion_r2078948256
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v5]
> This sketch shows how "Stable Updaters" can be used to create stable > computations of `@Stable` fields. Only one updater is needed per class, > similar to `AtomicIntegerFieldUpdater`. Per Minborg 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 15 additional commits since the last revision: - Add low level variants - Merge branch 'master' into stable-updaters - Reformat - Revert changes in public classes - Add a method handle based field updater - Fix raw long updater under 32-bit mode - Address comments - Document field alignment assumption - Add unhecked call test - Move raw factories into inner class Raw - ... and 5 more: https://git.openjdk.org/jdk/compare/de41c1f3...6342fbd8 - Changes: - all: https://git.openjdk.org/jdk/pull/25040/files - new: https://git.openjdk.org/jdk/pull/25040/files/4a42b271..6342fbd8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=03-04 Stats: 14466 lines in 388 files changed: 9105 ins; 3251 del; 2110 mod Patch: https://git.openjdk.org/jdk/pull/25040.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25040/head:pull/25040 PR: https://git.openjdk.org/jdk/pull/25040
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v4]
On Wed, 7 May 2025 11:54:09 GMT, David Beaumont wrote: >> Per Minborg has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Reformat >> - Revert changes in public classes > > test/jdk/java/lang/StableValue/StableFieldUpdaterExampleTest.java line 117: > >> 115: private static final ToIntFunction HASH_UPDATER = >> 116: StableFieldUpdater.ofInt(LazyFoo.class, "hash", >> 117: l -> Objects.hash(l.bar, l.baz), -1); > > As someone unfamiliar with this, I'm given to ask "What's the -1 for?" > > I *think* I can intuit that it's a value to be used *if* the hashcode > actually ends up calculating zero (so any random bit pattern would (should?) > be equally useful? > > In example code if feels like this is not quite explaining itself enough to > teach someone about its usage. For example (if it's what I think) it can't > actually be passed as zero. I've added some verbiage around this now. Let me know if it can be improved further. - PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077688537
Re: RFR: 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException [v9]
On Wed, 7 May 2025 11:34:08 GMT, Jaikiran Pai 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. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > move apiNote text to deprecated text Thanks! I prefer the new version. - Marked as reviewed by dfuchs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25031#pullrequestreview-2821467164
Re: RFR: 8355578: [java.net] Use @requires tag instead of exiting based on "os.name" property value [v2]
On Fri, 2 May 2025 09:15:24 GMT, Volkan Yazici wrote: >> Replaces `os.name` checks in tests with `@requires`. This prevents these >> tests from being run, and superficial results being generated, on >> unnecessary platforms. >> >> Note that `os.name` checks are replaced with `os.family` instead. > > Volkan Yazici has updated the pull request incrementally with one additional > commit since the last revision: > > Document `@requires` rationale with `@comment` > > Co-authored-by: Daniel Fuchs <67001856+df...@users.noreply.github.com> @vy Your change (at version 242f0787b26361e3679b57e0f5f5432dfc3a130d) is now ready to be sponsored by a Committer. - PR Comment: https://git.openjdk.org/jdk/pull/24997#issuecomment-2858301209
Integrated: 8355578: [java.net] Use @requires tag instead of exiting based on "os.name" property value
On Fri, 2 May 2025 08:26:48 GMT, Volkan Yazici wrote: > Replaces `os.name` checks in tests with `@requires`. This prevents these > tests from being run, and superficial results being generated, on unnecessary > platforms. > > Note that `os.name` checks are replaced with `os.family` instead. This pull request has now been integrated. Changeset: 60a4594b Author:Volkan Yazici Committer: Daniel Fuchs URL: https://git.openjdk.org/jdk/commit/60a4594b9f9acd82ef3ff22fc6a2df238dd981b9 Stats: 74 lines in 6 files changed: 11 ins; 55 del; 8 mod 8355578: [java.net] Use @requires tag instead of exiting based on "os.name" property value Reviewed-by: dfuchs - PR: https://git.openjdk.org/jdk/pull/24997
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v2]
On Tue, 6 May 2025 15:34:11 GMT, Daniel Fuchs wrote: >> Per Minborg has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix raw long updater under 32-bit mode > > src/java.base/share/classes/java/net/URI.java line 558: > >> 556: >> 557: // Used reflectively by HASH_UPDATER >> 558: @Stable > > What are the effect of this change on startup time? I remember that @cl4es > made a number of changes to URI to improve startup. Maybe this change should > be dropped out of this PR for now. Yes. I think we should remove these public changes and maybe come back in a separate PR with those. - PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077189936
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v4]
On Tue, 6 May 2025 12:23:44 GMT, Chen Liang wrote: >> This would require another implementation to be written. Maybe we can add >> that later? > > I feel like declaring a function is declaring redundant classes when we can > just reuse a DirectMethodHandle pointing to an actual method. Also the zero > alternative should be handled by the provided computing function/method > instead of handled by this utility. There is an MH variant now. The use of the zero alternative is optional, as if you put in `0`, it will have no effect. - PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077372099
Re: RFR: 7046003: Default value of Authenticator.getRequestingURL() is not specified [v2]
On Wed, 7 May 2025 15:58:29 GMT, Chen Liang wrote: > CCC is a legacy internal process only present for archival purposes. I think > we should withdraw that CCC and remove the "csr of" link so we can create a > new CSR. @liach - Old CSR withdrawn and new CSR created - JDK-8356410, thank you - PR Comment: https://git.openjdk.org/jdk/pull/25097#issuecomment-2859187855
Re: RFR: 8341346: Add support for exporting TLS Keying Material [v7]
On Wed, 7 May 2025 05:47:30 GMT, Bradford Wetmore wrote:
>> Adds the RFC 5705/8446 TLS Key Exporters API/implementation to JSSE/SunJSSE
>> respectively.
>>
>> CSR is underway.
>>
>> Tests include new unit tests for TLSv1-1.3. Will run tier1-2, plus the JCK
>> API (jck:api/java_security jck:api/javax_crypto jck:api/javax_net
>> jck:api/javax_security jck:api/org_ietf jck:api/javax_xml/crypto)
>
> Bradford Wetmore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Updated to use the upcoming KDF (still in preview) + bits of JDK-8353578
> for compilation)
src/java.base/share/classes/javax/net/ssl/ExtendedSSLSession.java line 169:
> 167:
> 168: /**
> 169: * Generate Exported Key Material (EKM) calculated according to the
s/Key/Keying/
src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 1808:
> 1806: String label, byte[] context, int length) throws
> SSLKeyException {
> 1807: byte[] bytes =
> 1808: exportKeyingMaterialKey(label, context,
> length).getEncoded();
In PKCS #11, calling `deriveKey(...).getEncoded()` is not equivalent to
`deriveData()`. It's quite likely that `deriveKey` returns an un-extractable
key, but `deriveData` using the exact same input returns the keying material.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24976#discussion_r2078023812
PR Review Comment: https://git.openjdk.org/jdk/pull/24976#discussion_r2078022859
Re: RFR: 8341346: Add support for exporting TLS Keying Material [v7]
On Wed, 7 May 2025 05:47:30 GMT, Bradford Wetmore wrote: >> Adds the RFC 5705/8446 TLS Key Exporters API/implementation to JSSE/SunJSSE >> respectively. >> >> CSR is underway. >> >> Tests include new unit tests for TLSv1-1.3. Will run tier1-2, plus the JCK >> API (jck:api/java_security jck:api/javax_crypto jck:api/javax_net >> jck:api/javax_security jck:api/org_ietf jck:api/javax_xml/crypto) > > Bradford Wetmore has updated the pull request incrementally with one > additional commit since the last revision: > > Updated to use the upcoming KDF (still in preview) + bits of JDK-8353578 > for compilation) src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 1694: > 1692: > 1693: // ...now the final expand. > 1694: SecretKey key = hkdf.deriveKey(label, PKCS #11 is picky about key algorithm names and I'm not sure if `label` is always accepted. The KDF API has the algorithm in the method arguments so it's left to user to specify one. I'm not sure how the export keying material will be used. If it is used in encryption, the algorithm may need to be something like "AES". - PR Review Comment: https://git.openjdk.org/jdk/pull/24976#discussion_r2078033799
Re: RFR: 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException [v9]
> 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. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: move apiNote text to deprecated text - Changes: - all: https://git.openjdk.org/jdk/pull/25031/files - new: https://git.openjdk.org/jdk/pull/25031/files/4cbe5178..77a332bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25031&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25031&range=07-08 Stats: 16 lines in 1 file changed: 4 ins; 10 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/25031.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25031/head:pull/25031 PR: https://git.openjdk.org/jdk/pull/25031
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v4]
On Wed, 7 May 2025 09:23:45 GMT, Per Minborg wrote: >> This sketch shows how "Stable Updaters" can be used to create stable >> computations of `@Stable` fields. Only one updater is needed per class, >> similar to `AtomicIntegerFieldUpdater`. > > Per Minborg has updated the pull request incrementally with two additional > commits since the last revision: > > - Reformat > - Revert changes in public classes The MH+VH form should be much more lightweight: MH and VH are just MemberName references. I wonder if we can advertise the MH+VH version as an indy BSM: public static CallSite lazyAccessor(Lookup lookup, String unused, MethodType callType, VarHandle field, MethodHandle computer) The CallSite would be constant, with `StableIntFieldUpdaterVarHandle.applyAsInt` bound to the specific updater. This way, we can even defer the creation of the updater. - PR Comment: https://git.openjdk.org/jdk/pull/25040#issuecomment-2858487803
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v6]
On Wed, 7 May 2025 14:58:33 GMT, Per Minborg wrote:
>> This sketch shows how "Stable Updaters" can be used to create stable
>> computations of `@Stable` fields. Only one updater is needed per class,
>> similar to `AtomicIntegerFieldUpdater`.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add convenience methods and documentations
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
line 257:
> 255: check(accessor, int.class);
> 256: Objects.requireNonNull(underlying);
> 257: var adaptedUnderlying = checkAndAdapt(underlying, int.class);
Suggestion:
// Implicit null checks:
check(accessor, int.class);
var adaptedUnderlying = checkAndAdapt(underlying, int.class);
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
line 311:
> 309: check(accessor, long.class);
> 310: Objects.requireNonNull(underlying);
> 311: var adaptedUnderlying = checkAndAdapt(underlying, long.class);
Suggestion:
// Implicit null checks:
check(accessor, long.class);
var adaptedUnderlying = checkAndAdapt(underlying, long.class);
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
line 588:
> 586:
> 587: private static MethodHandle checkAndAdapt(MethodHandle underlying,
> Class returnType) {
> 588: final var underlyingType = underlying.type();
Suggestion:
// Implicit null check
final var underlyingType = underlying.type();
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077860284
PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077860770
PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077861275
Re: RFR: 7046003: Default value of Authenticator.getRequestingURL() is not specified [v2]
On Wed, 7 May 2025 14:39:35 GMT, kieran-farrell wrote: >> Clarificatiion of spec to outline that Authenticator.getRequestingURL() >> returns null in the event that the corresponding request does not specify a >> URL. > > kieran-farrell has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes Need a small CSR to record this API specification update. You can create the CSR after the text has settled down. - PR Comment: https://git.openjdk.org/jdk/pull/25097#issuecomment-2858939201
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v7]
On Wed, 7 May 2025 15:03:57 GMT, Per Minborg wrote: >> This sketch shows how "Stable Updaters" can be used to create stable >> computations of `@Stable` fields. Only one updater is needed per class, >> similar to `AtomicIntegerFieldUpdater`. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Reformat `StableFieldUpdater.checkAndAdapt(…)` also performs an implicit null‑check: - PR Review: https://git.openjdk.org/jdk/pull/25040#pullrequestreview-2822161491
Re: RFR: 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException [v8]
On Tue, 6 May 2025 15:22:02 GMT, Daniel Fuchs wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Michael's review suggestions
>
> src/java.base/share/classes/java/net/Socket.java line 390:
>
>> 388: * The {@code stream} parameter provided a way in early JDK releases
>> 389: * to create a {@code Socket} that used a datagram socket. This
>> feature
>> 390: * 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 streammust 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:
https://github.com/user-attachments/assets/897e581e-28c4-424c-b3f9-3bce4591cb7e";
/>
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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25031#discussion_r2077324431
Re: RFR: 8353440: Disable FTP fallback for non-local file URLs by default [v14]
On Fri, 2 May 2025 20:32:27 GMT, Eirik Bjørsnøs wrote: >> Please help review this PR which disables the unspecified but long-standing >> feature where an `FtpURLConnection` is opened as a fallback for non-local >> file URLs. >> >> Before this change, if a file URL has a non-local host component, say >> `file://remotehost/folder/data.txt`, then the implementation would attempt >> opening an FTP connection to `remotehost`. After this change, such URLs will >> be rejected with a `MalformedURLException`, unless the FTP fallback feature >> is explicitly re-enabled via a system property. >> >> This change was initially discussed here: >> https://mail.openjdk.org/pipermail/net-dev/2025-March/025988.html >> >> See the above discussion and CSR draft JDK-8354678 for the motivation for >> this change. >> >> This PR: >> >> * Changes file URL `Handler::openConnection` implementation for unix/windows >> to throw `MalformedURLException`, unless the FTP fallback feature is >> explicitly enabled by configuration. >> * Introduces a new system property `jdk.net.file.ftpenabled` which when set >> to `true` re-enables the feature. >> * Documents the new property in `net-properties.html` >> * Updates the existing test `NonLocalFtpFallback` to enable the feature via >> said system property. >> * Adds a new test `NonLocalFtpFallbackDisabled` verifying that a >> `MalformedURLException` is thrown by default for a non-local URL host >> component. >> * Moves testing of exceptional behavior in FtpConnection when using >> non-local file URLs with FTP fallback enabled from `OpenStream` to >> `NonLocalFtpFallback` >> >> I have added a Release Note as a subtask in the JBS issue, this also needs a >> review. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove extra space > > Co-authored-by: Shaojin Wen Thanks for your patient help Daniel! I’ll let this behavioral change in an ancient and dusty corner of the JDK linger for another 24h, just out of caution for any late reviewers. - PR Comment: https://git.openjdk.org/jdk/pull/24657#issuecomment-2858282788
Re: RFR: 7046003: Default value of Authenticator.getRequestingURL() is not specified [v2]
On Wed, 7 May 2025 14:39:35 GMT, kieran-farrell wrote: >> Clarificatiion of spec to outline that Authenticator.getRequestingURL() >> returns null in the event that the corresponding request does not specify a >> URL. > > kieran-farrell has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes > /csr needed > > Need a small CSR to record this API specification update. You can create the > CSR after the text has settled down. @liach I populated the previously created CSR CCC-7046003 as linked to the bug. Would you be okay to review? - PR Comment: https://git.openjdk.org/jdk/pull/25097#issuecomment-2859135098
Re: RFR: 7046003: Default value of Authenticator.getRequestingURL() is not specified [v2]
On Wed, 7 May 2025 14:39:35 GMT, kieran-farrell wrote: >> Clarificatiion of spec to outline that Authenticator.getRequestingURL() >> returns null in the event that the corresponding request does not specify a >> URL. > > kieran-farrell has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes CCC is a legacy internal process only present for archival purposes. I think we should withdraw that CCC and remove the "csr of" link so we can create a new CSR. - PR Comment: https://git.openjdk.org/jdk/pull/25097#issuecomment-2859150050
Re: RFR: 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException [v8]
On Wed, 7 May 2025 10:29:44 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/java/net/Socket.java line 390:
>>
>>> 388: * The {@code stream} parameter provided a way in early JDK
>>> releases
>>> 389: * to create a {@code Socket} that used a datagram socket. This
>>> feature
>>> 390: * 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 streammust 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:
>
> src="https://github.com/user-attachments/assets/897e581e-28c4-424c-b3f9-3bce4591cb7e";
> />
>
>
> 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".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25031#discussion_r2077331080
Re: RFR: 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException [v8]
On Wed, 7 May 2025 10:34:07 GMT, Alan Bateman wrote: > 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". - PR Review Comment: https://git.openjdk.org/jdk/pull/25031#discussion_r2077336378
Re: RFR: 8356154: Respecify java.net.Socket constructors that allow creating UDP sockets to throw IllegalArgumentException [v8]
On Wed, 7 May 2025 11:16:48 GMT, Daniel Fuchs wrote: >>> 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/25031#discussion_r2077420523
Re: RFR: 7046003: Default value of Authenticator.getRequestingURL() is not specified [v2]
> Clarificatiion of spec to outline that Authenticator.getRequestingURL() > returns null in the event that the corresponding request does not specify a > URL. kieran-farrell has updated the pull request incrementally with one additional commit since the last revision: revert changes - Changes: - all: https://git.openjdk.org/jdk/pull/25097/files - new: https://git.openjdk.org/jdk/pull/25097/files/4d6eb730..6f12e2b0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25097&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25097&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/25097.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25097/head:pull/25097 PR: https://git.openjdk.org/jdk/pull/25097
Re: RFR: 8356395: Spec needs to be clarified for InterfaceAddress.getBroadcast() method
On Wed, 7 May 2025 14:02:52 GMT, kieran-farrell wrote:
> Spec currently suggests that only IPv6 addresses can return null for
> InterfaceAddress.getBroadcast(). Clarifying spec to state that certain IPv4
> address such as the loopback address do not support broadcasting and can
> therefore also return null.
src/java.base/share/classes/java/net/InterfaceAddress.java line 67:
> 65: *
> 66: * Certain IPv4 addresses, such as the loopback address, do not
> support
> 67: * broadcasting and will also result in {@code null} being returned.
Suggestion:
* Certain network interfaces, such as the loopback interface, do not
support
* broadcasting and will also return {@code null}.
Maybe the class level API documentation should also be updated something like:
* This class represents a Network Interface address. In short it's an
* IP address, a subnet mask as well as a broadcast address when the address is
* IPv4 and the interface supports broadcasting.
* An IP address and a network prefix length in the case
* of IPv6 address.
It would be good to have @Michael-Mc-Mahon approve the wording.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25095#discussion_r2077800913
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v6]
> This sketch shows how "Stable Updaters" can be used to create stable > computations of `@Stable` fields. Only one updater is needed per class, > similar to `AtomicIntegerFieldUpdater`. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Add convenience methods and documentations - Changes: - all: https://git.openjdk.org/jdk/pull/25040/files - new: https://git.openjdk.org/jdk/pull/25040/files/6342fbd8..a01ba9ab Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=04-05 Stats: 107 lines in 2 files changed: 106 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/25040.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25040/head:pull/25040 PR: https://git.openjdk.org/jdk/pull/25040
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v7]
> This sketch shows how "Stable Updaters" can be used to create stable > computations of `@Stable` fields. Only one updater is needed per class, > similar to `AtomicIntegerFieldUpdater`. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Reformat - Changes: - all: https://git.openjdk.org/jdk/pull/25040/files - new: https://git.openjdk.org/jdk/pull/25040/files/a01ba9ab..de8e2387 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=05-06 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/25040.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25040/head:pull/25040 PR: https://git.openjdk.org/jdk/pull/25040
Re: RFR: 8355578: [java.net] Use @requires tag instead of exiting based on "os.name" property value [v2]
On Wed, 7 May 2025 11:29:11 GMT, Daniel Fuchs wrote: >> Volkan Yazici has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Document `@requires` rationale with `@comment` >> >> Co-authored-by: Daniel Fuchs <67001856+df...@users.noreply.github.com> > > test/jdk/java/net/MulticastSocket/PromiscuousIPv6.java line 27: > >> 25: * @test >> 26: * @bug 8215294 >> 27: * @requires os.family == "linux" & !(os.version ~= "3\\.10\\.0.*") > > Would that match 13.10.01? No. Adding `@requires os.family ~= "inux"` causes a test to be discarded on my (Linux) system. That is, the pattern is implicitly wrapped with `^` and `$` directives. Whereas `@requires os.family ~= "linux"` causes the test to get executed. - PR Review Comment: https://git.openjdk.org/jdk/pull/24997#discussion_r2077450781
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v4]
On Wed, 7 May 2025 09:23:45 GMT, Per Minborg wrote: >> This sketch shows how "Stable Updaters" can be used to create stable >> computations of `@Stable` fields. Only one updater is needed per class, >> similar to `AtomicIntegerFieldUpdater`. > > Per Minborg has updated the pull request incrementally with two additional > commits since the last revision: > > - Reformat > - Revert changes in public classes Starting to look nicely idiomatic. If I saw this I would however want to add a thin wrapper specific for hash codes where the "zero replacement" and generic types are more hidden. * HashCoder.forIntField(Foo.class, "hash", ...) * HashCoder.forLongField(...) I like that there's an annotation on the field now though! test/jdk/java/lang/StableValue/StableFieldUpdaterExampleTest.java line 83: > 81: > 82: > 83: static // Intentionally in unblessed order to allow the example to > look nice Huh, not seen this before ... interesting. test/jdk/java/lang/StableValue/StableFieldUpdaterExampleTest.java line 117: > 115: private static final ToIntFunction HASH_UPDATER = > 116: StableFieldUpdater.ofInt(LazyFoo.class, "hash", > 117: l -> Objects.hash(l.bar, l.baz), -1); As someone unfamiliar with this, I'm given to ask "What's the -1 for?" I *think* I can intuit that it's a value to be used *if* the hashcode actually ends up calculating zero (so any random bit pattern would (should?) be equally useful? In example code if feels like this is not quite explaining itself enough to teach someone about its usage. For example (if it's what I think) it can't actually be passed as zero. test/jdk/java/lang/StableValue/StableFieldUpdaterExampleTest.java line 149: > 147: private static final ToLongFunction > HASH_UPDATER = > 148: StableFieldUpdater.ofLong(LazySpecifiedFoo.class, "hash", > 149: l -> (l.bar == null && l.baz == null) ? 0 : > Objects.hash(l.bar, l.baz), 1L << 32); Maybe for an example, use a (private?) static method reference here `calculateHash`. It helps visually distinguish plumbing and business logic IMO. - PR Review: https://git.openjdk.org/jdk/pull/25040#pullrequestreview-2821463165 PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077457809 PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077457604 PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077460209
Re: RFR: 8341346: Add support for exporting TLS Keying Material [v8]
> Adds the RFC 5705/8446 TLS Key Exporters API/implementation to JSSE/SunJSSE > respectively. > > CSR is underway. > > Tests include new unit tests for TLSv1-1.3. Will run tier1-2, plus the JCK > API (jck:api/java_security jck:api/javax_crypto jck:api/javax_net > jck:api/javax_security jck:api/org_ietf jck:api/javax_xml/crypto) Bradford Wetmore has updated the pull request incrementally with one additional commit since the last revision: More Codereview comments - Changes: - all: https://git.openjdk.org/jdk/pull/24976/files - new: https://git.openjdk.org/jdk/pull/24976/files/c6baa83b..2e5f5342 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24976&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24976&range=06-07 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/24976.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24976/head:pull/24976 PR: https://git.openjdk.org/jdk/pull/24976
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v3]
> This sketch shows how "Stable Updaters" can be used to create stable > computations of `@Stable` fields. Only one updater is needed per class, > similar to `AtomicIntegerFieldUpdater`. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Add a method handle based field updater - Changes: - all: https://git.openjdk.org/jdk/pull/25040/files - new: https://git.openjdk.org/jdk/pull/25040/files/52e96c52..83ce9ac1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25040&range=01-02 Stats: 95 lines in 2 files changed: 95 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/25040.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25040/head:pull/25040 PR: https://git.openjdk.org/jdk/pull/25040
Re: RFR: 8356255: Add Stable Field Updaters to allow efficient lazy field evaluations [v3]
On Wed, 7 May 2025 07:46:45 GMT, Per Minborg wrote:
>> This sketch shows how "Stable Updaters" can be used to create stable
>> computations of `@Stable` fields. Only one updater is needed per class,
>> similar to `AtomicIntegerFieldUpdater`.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add a method handle based field updater
src/java.base/share/classes/jdk/internal/lang/stable/StableFieldUpdater.java
line 173:
> 171: if (!underlying.type().parameterType(0).equals(Object.class)) {
> 172: underlying =
> underlying.asType(underlying.type().changeParameterType(0, Object.class));
> 173: }
Suggestion:
var type = underlying.type();
if (type.returnType() != int.class || type.parameterCount() != 1) {
throw new IllegalArgumentException("Illegal underlying function: "
+ underlying);
}
if (!type.parameterType(0).equals(Object.class)) {
underlying = underlying.asType(type.changeParameterType(0,
Object.class));
}
underlying.type() is used 4 times, local variables should be used
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25040#discussion_r2077031643
