Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat [v4]
On Tue, 14 Jun 2022 12:18:52 GMT, Matthias Baesken wrote: >> When trying to construct an LdapURL object with a bad input string (in this >> example the _ in ad_jbs is causing issues), and not using >> the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we >> run into the exception below : >> >> import com.sun.jndi.ldap.LdapURL; >> >> String url = "ldap://ad_jbs.ttt.net:389/xyz";; // bad input string containing >> _ >> LdapURL ldapUrl = new LdapURL(url); >> >> >> java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest >> Exception in thread "main" javax.naming.NamingException: Cannot parse url: >> ldap://ad_jbs.ttt.net:389/xyz [Root exception is >> java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389] >> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115) >> at LdapParseUrlTest.main(LdapParseUrlTest.java:9) >> Caused by: java.net.MalformedURLException: unsupported authority: >> ad_jbs.ttt.net:389 >> at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367) >> at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230) >> at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174) >> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105) >> >> I would like to add the host and port info to the exception (in the example >> it is host:port of URI:null:-1] ) so that it is directly visible that the >> input caused the construction of a URI >> with "special"/problematic host and port values. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > avoid very long line The last changes LGTM. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: JDK-8288207: Enhance MalformedURLException in Uri.parseCompat
On Fri, 10 Jun 2022 12:16:17 GMT, Matthias Baesken wrote:
> When trying to construct an LdapURL object with a bad input string (in this
> example the _ in ad_jbs is causing issues), and not using
> the backward compatibility flag -Dcom.sun.jndi.ldapURLParsing="legacy" we run
> into the exception below :
>
> import com.sun.jndi.ldap.LdapURL;
>
> String url = "ldap://ad_jbs.ttt.net:389/xyz";; // bad input string containing _
> LdapURL ldapUrl = new LdapURL(url);
>
>
> java --add-opens java.naming/com.sun.jndi.ldap=ALL-UNNAMED LdapParseUrlTest
> Exception in thread "main" javax.naming.NamingException: Cannot parse url:
> ldap://ad_jbs.ttt.net:389/xyz [Root exception is
> java.net.MalformedURLException: unsupported authority: ad_jbs.ttt.net:389]
> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:115)
> at LdapParseUrlTest.main(LdapParseUrlTest.java:9)
> Caused by: java.net.MalformedURLException: unsupported authority:
> ad_jbs.ttt.net:389
> at java.naming/com.sun.jndi.toolkit.url.Uri.parseCompat(Uri.java:367)
> at java.naming/com.sun.jndi.toolkit.url.Uri.parse(Uri.java:230)
> at java.naming/com.sun.jndi.toolkit.url.Uri.init(Uri.java:174)
> at java.naming/com.sun.jndi.ldap.LdapURL.(LdapURL.java:105)
>
> I would like to add the host and port info to the exception (in the example
> it is host:port of URI:null:-1] ) so that it is directly visible that the
> input caused the construction of a URI
> with "special"/problematic host and port values.
`URISyntaxException`/`MalformedURLException` usually contains the whole URL -
so in this case, because we're parsing a URL, I believe the added information
would not leak more sensitive data - especially since I'd expect URI.getHost()
to be always `null` and `URI.getPort()` to be always `-1` in this case.
That is - this exception is thrown when the authority is parsed as a reg_name,
as opposed to server-base, because the provided host name (or what looks like a
host name) contains a character that is not allowed by java.net.URI in a host
name.
jshell> URI.create("ldap://a_b.com:389/foo";);
$1 ==> ldap://a_b.com:389/foo
jshell> $1.getAuthority()
$2 ==> "a_b.com:389"
jshell> $1.getHost()
$3 ==> null
As a point of comparison, here is what URISyntaxException looks like if the
authority contains a character which is not legal at all in authority:
jshell> new URI("ldap://a_%b.com:389/foo";);
| Exception java.net.URISyntaxException: Malformed escape pair at index 9:
ldap://a_%b.com:389/foo
|at URI$Parser.fail (URI.java:2973)
I agree we should wait for someone from security-dev to chime in though.
I might question whether the added "null:-1" information is really helpful, or
just as confusing however.
-
PR: https://git.openjdk.org/jdk/pull/9126
Re: RFR: 8287766: Unnecessary Vector usage in LdapClient
On Sun, 29 May 2022 20:22:55 GMT, Andrey Turbanov wrote: > In [JDK-8273098](https://bugs.openjdk.java.net/browse/JDK-8273098) I missed > one place, where Vector could be replaced with ArrayList. > Usage of thread-safe collection `Vector` is unnecessary. It's recommended to > use `ArrayList` if a thread-safe implementation is not needed. LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8940
Re: RFR: 8287442: Reduce list to array conversions in java.lang.invoke.MethodHandles [v2]
On Thu, 2 Jun 2022 13:59:50 GMT, Сергей Цыпанов wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Review comments, eagerly convert sooner in tryFinally > > src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 5462: > >> 5460: Objects.requireNonNull(target); >> 5461: Objects.requireNonNull(newTypes); >> 5462: return dropArgumentsToMatch(target, skip, newTypes.toArray(new >> Class[0]).clone(), pos, false); > > Do we really need to clone an array returned from `List.toArray()`? As far as > I know from the JavaDoc of `List` if the passed array is not long enough to > include all the items then the new array must be allocated. Here we always > pass empty arrays, so the new ones are returned from `toArray()` method and > we don't need `clone()`, right? The clone is needed - as the `List>` may be a custom implementation of List - so you cannot make any assumption on the concrete implementation of `toArray`. - PR: https://git.openjdk.java.net/jdk/pull/8923
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Fri, 27 May 2022 18:40:32 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > do it as naotoj said Changes to `net` and `http` look good. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8287672: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run
On Wed, 1 Jun 2022 15:41:57 GMT, Rob McKenna wrote:
> Test change to silently pass if the test environment encounters a
> NoRouteToHostException. These are intermittent at the moment but I will
> attempt to find an alternative to the use of example.com in some follow up
> work.
Marked as reviewed by dfuchs (Reviewer).
test/jdk/com/sun/jndi/ldap/LdapPoolTimeoutTest.java line 124:
> 122: System.err.println("MSG RTE: " + msg);
> 123: // assertCompletion may wrap a CommunicationException in an
> RTE
> 124: assertTrue(msg != null);
LGTM. Nit: I believe there is an `assertNotNull()`
-
PR: https://git.openjdk.java.net/jdk/pull/8974
Re: RFR: 8281695: jtreg test com/sun/jndi/ldap/LdapPoolTimeoutTest.java fails intermittently in nightly run
On Fri, 27 May 2022 15:24:46 GMT, Rob McKenna wrote: > Test change to silently pass if the test environment encounters a > NoRouteToHostException. These are intermittent at the moment but I will > attempt to find an alternative to the use of example.com in some follow up > work. test/jdk/com/sun/jndi/ldap/LdapTimeoutTest.java line 135: > 133: failedCount++; > 134: cause.printStackTrace(System.out); > 135: } Maybe this could be simplified as: Throwable cause = e.getCause(); // don't fail if we get NoRouteToHostException if (cause instanceof NoRouteToHostException) continue; if (cause != null && cause.getCause() != null) cause = cause.getCause(); if (cause instanceof NoRouteToHostException) continue; failedCount++; cause.printStackTrace(System.out); - PR: https://git.openjdk.java.net/jdk/pull/8925
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V) [v4]
On Fri, 6 May 2022 22:05:35 GMT, liach wrote:
>> Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare
>> values by identity. Updated API documentation of these two methods
>> ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object)))
>> to mention such behavior.
>
> liach 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 six additional commits since the
> last revision:
>
> - Move tests
> - Merge branch 'master' into fix/identityhashmap-default
> - Fix assertions
> - Revamp test and changes. Let ci run the tests
> - Fix indent
> - 8178355: IdentityHashMap uses identity-based comparison for values
> everywhere except remove(K,V) and replace(K,V,V)
src/java.base/share/classes/java/util/IdentityHashMap.java line 1392:
> 1390: * and {@code value == v}, then this method removes the mapping
> 1391: * for this key and returns {@code true}; otherwise it returns
> 1392: * {@code false}.
The API documentation of containsKey() and containsValue() should probably be
updated to mention reference equality too. This doesn't need to be carried out
in this PR, but maybe a new issue should be logged to double check the
completeness of the IdentityHashMap API documentation and see where adding some
similar text makes sense.
-
PR: https://git.openjdk.java.net/jdk/pull/8259
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote:
> Hi,
>
> May I have this test update reviewed?
>
> The ForceGC could be enhanced by using smaller wait/sleep time, and shared
> cleaner.
>
> Thanks,
> Xuelei
LGTM - but it may be good to have an other reviewer (@mlchung ?)
test/lib/jdk/test/lib/util/ForceGC.java line 50:
> 48: for (int i = 0; i < 100; i++) {
> 49: System.gc();
> 50: System.out.println("doIt() iter: " + iter + ", gc " + i);
Maybe the trace should be printed only if `(i % 10 == 0) && (iter % 100 == 0)`
to avoid having too many traces in the log?
-
Marked as reviewed by dfuchs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator)
On Thu, 19 May 2022 13:05:54 GMT, Alan Bateman wrote:
> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>
> This is a non-final API that provides a gentle on-ramp to structure a task as
> a family of concurrent subtasks, and to coordinate the subtasks as a unit.
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
line 54:
> 52:
> 53: /**
> 54: * A basic API for structured concurrency. StructuredTaskScope
> supports cases
Should StructuredTaskScope in this class-level API doc comment be surrounded by
`{@code }` to appear in code font?
-
PR: https://git.openjdk.java.net/jdk/pull/8787
Re: RFR: 8284209: Replace remaining usages of 'a the' in source code
On Wed, 18 May 2022 14:46:42 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > It's the last issue in the series, and it still touches different areas of > the code. Logging/JNDI OK - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8771
Re: RFR: 8286744: failure_handler: dmesg command on macos fails to collect data due to permission issues [v2]
On Fri, 13 May 2022 14:36:02 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which proposes to fix the failure >> handler command `dmesg` on macOS? >> >> As noted in the JBS issue, the command currently fails with permission >> error. The commit in this PR uses `sudo` as suggested in the man pages of >> that command. >> >> Tested locally on macOS by running: >> >> >> cd test/failure_handler >> make clean >> make test >> >> Then checked the generated environment.html files for the dmesg command >> output. Looks fine after this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > copyright year LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8703
Re: RFR: 8286390: Address possibly lossy conversions in jdk.incubator.foreign moved to java.base [v2]
On Fri, 13 May 2022 12:06:45 GMT, Jorn Vernee wrote: >> Address possible lossy conversion warning in `ProgrammableInvoker`. >> >> Testing: `run-test-jdk_foreign`, testing with patch from >> https://github.com/openjdk/jdk/pull/8599 to see if the warning is gone. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Use plain int cast instead Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8697
Re: RFR: 8286390: Address possibly lossy conversions in jdk.incubator.foreign moved to java.base
On Fri, 13 May 2022 10:04:36 GMT, Jorn Vernee wrote:
> Address possible lossy conversion warning in `ProgrammableInvoker`.
>
> Testing: `run-test-jdk_foreign`, testing with patch from
> https://github.com/openjdk/jdk/pull/8599 to see if the warning is gone.
src/java.base/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java
line 215:
> 213: for (int i = 0; i < highLevelType.parameterCount(); i++) {
> 214: List bindings = callingSequence.argumentBindings(i);
> 215: argInsertPos +=
> Math.toIntExact(bindings.stream().filter(Binding.VMStore.class::isInstance).count())
> + 1;
My gut feeling in this case would be that it's a bit strange to use
`Math.toIntExact` to do a safe cast when you don't do `Math.addExact` to ensure
that the result of the addition will not overflow. I wonder if a simple cast
wouldn't be more appropriate - unless you really think that you might have more
than Integer.MAX_VALUE bindings (which I doubt :-) ). But that's just my
feeling...
-
PR: https://git.openjdk.java.net/jdk/pull/8697
Re: RFR: 8286393: Address possibly lossy conversions in java.rmi
On Thu, 12 May 2022 16:47:43 GMT, Roger Riggs wrote: > Updates to modules java.rmi and java.smartcardio to remove warnings about > lossy-conversions introduced by PR#8599. > > Explicit casts are inserted where implicit casts occur. > > 8286393: Address possibly lossy conversions in java.rmi > 8286388: Address possibly lossy conversions in java.smartcardio Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8683
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to > the state previous to JDK-8160768, where an authentication failure stops from > trying other LDAP servers with the same credentials [1]. After JDK-8160768 we > have 2 possible loops to stop: the one that iterates over different URLs and > the one that iterates over different endpoints (after a DNS query that > returns multiple values). > > No test regressions observed in jdk/com/sun/jndi/ldap. > > -- > [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137 Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: HttpClient has no explicit way of releasing threads
On 10/05/2022 14:39, Remi Forax wrote: This is not a big problem but I have seen peaks with suddenly many, many threads (in test code) where many HttpClients were created for single use and I was wondering if it was ever considered to add a method for disposing the threads explicitly? You can change the Executor (it's one parameter of the builder) to use whatever executors you want so you can shutdown that executor as you want. This should fixed (1). Also once you update to Java 19/21, it seems a good scenario to test the executor that spawn virtual threads instead of platform threads. Some word of caution about shutting down the executor: If you know that the client is no longer used, and there are no requests in progress, what Rémi suggests should be fine. Otherwise shutting down the executor when the client is still in use could lead to undefined behaviour, including not being able to complete the CompletableFutures that have been returned by `sendAsync` - or which `send` calls have joined. This has been fixed in JDK 19 by JDK-8277969, but otherwise, and especially on previous versions of the JDK, you should make sure that all operations have terminated before shutting down the executor (even gracefully). Using virtual threads should be fine - as long as they are not pooled :-) best regards, -- daniel
Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]
On Tue, 10 May 2022 11:31:16 GMT, Andrey Turbanov wrote:
>> src/java.desktop/share/classes/javax/imageio/spi/ServiceRegistry.java line
>> 230:
>>
>>> 228: List l = new ArrayList<>();
>>> 229: for (Class c : categoryMap.keySet()) {
>>> 230: if (c.isInstance(provider)) {
>>
>> Can this be reached if `provider` is null? If yes there could be a change of
>> behaviour as the previous code would have thrown NPE.
>
> No. This method is called from 3 places, and there 3 null checks before the
> method call.
Thanks for double checking! LGTM then.
-
PR: https://git.openjdk.java.net/jdk/pull/7061
Re: HttpClient has no explicit way of releasing threads
Hi Rafael, On 09/05/2022 22:43, Rafael Winterhalter wrote: Hello, looking at thread dumps, I realized that the HttpClient implementation does not offer an explicit way to release its threads. Currently, the client: 1. maintains a cached thread pool with a retention size of 60 seconds. If many such clients are created for short lived application, these threads pile up. 2. has a selector thread that only shuts down if the outer "facade" reference is collected via a weak reference. If an application is not running GC, this can take a while. This is not a big problem but I have seen peaks with suddenly many, many threads (in test code) where many HttpClients were created for single use and I was wondering if it was ever considered to add a method for disposing the threads explicitly? I would consider it bad practice to create an HttpClient instance for single usage; Ideally a client should be reused - provided that the security context is the same. Creating an HttpClient involves creating a thread pool, a selector, a selector manager thread, potentially initializing an SSL context etc... WRT to adding a method for disposing the HttpClient explicitly, then yes - that's something that we could consider for a major release. It would need to be carefully specified - especially WRT what would be the effect of calling this method if some operations are still in progress. Asynchronously closing objects that are still in use is a notoriously thorny subject. We might need something equivalent to what is defined for executor services - that is - one variant that waits for all ongoing operations to terminate before closing, and one that abruptly aborts any on-going operation. Alternatively, it might be an option to add a method like HttpClient.shared() which would return a singleton HttpClient (created on the first call, collected if no reference is kept anymore but reused in the meantime) to address such scenarios. I can of course add a singleton in my test project but I find this a bit awkward as it is something to remember and to repeat in all applications we maintain. Therefore, it might be convenient to add such methods for tests that usually aim to be decoupled. An HttpClient is a kind of capability object so I don't think we want to have a shared client in the Java API. That's something that an application can easily implement at the application level if it makes sense for the application. A possibility to work around the thread peak issue is also for an application to configure its own thread executor on the HttpClient. If that makes sense for the application and if it is safe to do so the executor can also be shared between several client. It is then the responsibility of the application to shutdown the executor when the clients are no longer in use. Thanks for your consideration, best regards, Rafael best regards, -- daniel
Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]
On Thu, 31 Mar 2022 08:03:23 GMT, Andrey Turbanov wrote:
>> Method `Class.isAssignableFrom` is often used in form of:
>>
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>>
>> if (clazz.isInstance(obj)) {
>>
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>>
>> I tried to measure performance via JMH
>>
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>>
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>>
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>>
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>>
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>>
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>>
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>>
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>>
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>>
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>>
>> `isInstance` is a bit faster than `isAssignableFrom`
>>
>> Benchmark Mode Cnt Score Error Units
>> integerDouble_isAssignableFrom avgt5 1,173 ± 0,026 ns/op
>> integerDouble_isInstance avgt5 0,939 ± 0,038 ns/op
>> integerIntegerDouble_isAssignableFrom avgt5 2,106 ± 0,068 ns/op
>> numberIntegerDouble_isInstance avgt5 1,516 ± 0,046 ns/op
>> integerInteger_isAssignableFromavgt5 1,175 ± 0,029 ns/op
>> integerInteger_isInstance avgt5 0,886 ± 0,017 ns/op
>> numberDouble_isAssignableFrom avgt5 1,172 ± 0,007 ns/op
>> numberDouble_isInstanceavgt5 0,891 ± 0,030 ns/op
>> numberInteger_isAssignableFrom avgt5 1,169 ± 0,014 ns/op
>> numberInteger_isInstance avgt5 0,887 ± 0,016 ns/op
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8280035: Use Class.isInstance instead of Class.isAssignableFrom where
> applicable
> apply suggestion to avoid second Method.invoke call
src/java.desktop/share/classes/javax/imageio/spi/ServiceRegistry.java line 230:
> 228: List l = new ArrayList<>();
> 229: for (Class c : categoryMap.keySet()) {
> 230: if (c.isInstance(provider)) {
Can this be reached if `provider` is null? If yes there could be a change of
behaviour as the previous code would have thrown NPE.
-
PR: https://git.openjdk.java.net/jdk/pull/7061
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v9]
On Mon, 2 May 2022 13:01:40 GMT, Sibabrata Sahoo wrote: >> A new API to support replacing selective lines with desired content. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > 8285452: Tests updated LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 18:28:45 GMT, Weijun Wang wrote: >> Shouldn't the comparison be better implemented by the caller then, who will >> know whether spaces are important or not? That's why I had suggested taking >> a `Predicate` that could be called with each line removed, and the >> caller could interrupt the parsing by returning false when they detect a >> mismatch with what they expect. > > We can provide a more sophisticated `Function` replacer if we > want to let user to customize all the details. This time we still only want > them to be string literals. I agree we can keep the new lines inside, but > trimming on each line and the final block is still useful so caller does not > need to care about indentation and empty lines at both ends. OK - if you keep the internal new lines I have no objection. The API doc should however say that lines will be trimmed before comparing them. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 13:02:06 GMT, Weijun Wang wrote: >> Also calling trim() assumes that white spaces are not significant. This >> might not be the case in the general case (for instance - think of markdown >> files were leading spaces are significant). > > The comparison is intentionally made lax so the caller does not need to > provide the exact indentation or even new line characters. We think along > with `fromLine` and `toLine` this is enough to make sure we are not modifying > the wrong lines. Shouldn't the comparison be better implemented by the caller then, who will know whether spaces are important or not? That's why I had suggested taking a `Predicate` that could be called with each line removed, and the caller could interrupt the parsing by returning false when it detects a mismatch with what they expect. - PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:35:59 GMT, Daniel Fuchs wrote:
>> Sibabrata Sahoo has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8285452: updated to use lines()
>
> test/lib/jdk/test/lib/util/FileUtils.java line 394:
>
>> 392: var removed = "";
>> 393: for (int i = fromLine; i <= toLine; i++) {
>> 394: removed += lines.remove(fromLine - 1).trim();
>
> shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c" will
> be the same as concatenating lines "a" and "bc".
Also calling trim() assumes that white spaces are not significant. This might
not be the case in the general case (for instance - think of markdown files
were leading spaces are significant).
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Add a new test library API to replace a file content using FileUtils.java [v7]
On Fri, 29 Apr 2022 12:29:35 GMT, Sibabrata Sahoo wrote:
>> A new API to support replacing selective lines with desired content.
>
> Sibabrata Sahoo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8285452: updated to use lines()
test/lib/jdk/test/lib/util/FileUtils.java line 394:
> 392: var removed = "";
> 393: for (int i = fromLine; i <= toLine; i++) {
> 394: removed += lines.remove(fromLine - 1).trim();
shouldn't you insert a "\n" ? otherwise concatenating lines "ab" and "c" will
be the same as concatenating lines "a" and "bc".
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285915: failure_handler: gather the contents of /etc/hosts file
On Fri, 29 Apr 2022 11:28:32 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which addresses > https://bugs.openjdk.java.net/browse/JDK-8285915? > > With this change, the environment details collected by the failure handler > will now include the contents of the `/etc/hosts/` file, which can be useful > in certain cases when debugging network tests that fail. > > Testing done (on macOS): > > > cd test/failure_handler > make test > > Then verified that the generated environment.html had the `/etc/hosts` file > content Thanks for doing this Jaikiran! That should be helpful. Please get approval from someone from build-dev before integrating. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8466
Re: RFR: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8465
Re: [External] : Re: java.net.http.HttpClient do NOT support Digest authentication while HttpURLConnection do
Hi, Thanks for the pointers. > Just to mention a few: > https://urldefense.com/v3/__https://www.baeldung.com/java-9-http-client__;!!ACWV5N9M2RV99hQ!MbomkSmr-rXolK88z_JdINs3IEG8MTH_B7DHDq6wYgUeJQvZNGT7Iwb3yFswd57S1l85R53Yo_zq8zIvWdw$ > https://urldefense.com/v3/__https://developer.ibm.com/tutorials/java-theory-and-practice-3/__;!!ACWV5N9M2RV99hQ!MbomkSmr-rXolK88z_JdINs3IEG8MTH_B7DHDq6wYgUeJQvZNGT7Iwb3yFswd57S1l85R53Yo_zqgE8E3ik$ "An Authenticator is an object that negotiates credentials (HTTP authentication) for a connection. It provides different authentication schemes (such as basic or digest authentication)." I believe that's a misunderstanding: you will notice that the text talks about the Authenticator - not the HttpClient. But I do agree that this text is misleading (even WRT Authenticator). HttpURLConnection does support digest authentication out of the box. It was a choice we made to not support any authentication scheme out of the box, except Basic, in the new API. As I said Digest can be easily implemented at the application level if you need it, by handling the 401/407 responses at the application level. best regards, -- daniel
Re: java.net.http.HttpClient do NOT support Digest authentication while HttpURLConnection do
Hi,
java.net.http.HttpClient only supports Basic authentication out
of the box.
Which tutorials are claiming that Digest authentication is supported?
Can you send some links?
At the moment there is no plan to support digest authentication out of
the box. It can be easily implemented at the application level on top
of the existing APIs, by *not* registering an authenticator with
the client and dealing with 401/407 response from the application code.
best regards,
-- daniel
On 28/04/2022 23:40, Farkas Levente wrote:
Hi,
Even though many tutorial said that the new java.net.http.HttpClient
support Digest authentication it's not true:
https://github.com/openjdk/jdk/blob/master/src/java.net.http/share/classes/jdk/internal/net/http/AuthenticationFilter.java#L278
But at the same time HttpURLConnection support it through the simple:
---
Authenticator.setDefault(new Authenticator() {
@Override
protected PasswordAuthentication getPasswordAuthentication() {
return new PasswordAuthentication (
"username",
"password".toCharArray()
);
}
});
---
Is it a bug or you don't even plan to support it with HttpClient?
Regards.
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]
On Thu, 28 Apr 2022 01:34:19 GMT, Joe Darcy wrote: >> To enable more complete doclint checking (courtesy @jonathan-gibbons), >> please review this PR to add type-level @param tags where they are missing. >> >> To the maintainers of java.util.concurrent, those changes could be separated >> out in another bug if that would ease maintenance of that code. >> >> Making these library fixes is a blocker for correcting and expanding the >> doclint checks (JDK-8285496). >> >> I'll update copyright years before pushing. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to more review feedback. Thanks for the updates Joe. Your new wording looks good to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8410
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e Thanks for the refresh Alan! Things look good to me now. I have gone over java.io, the networking parts of java.nio, java.net, and the JMX related changes. For what I have reviewed, I believe this is good to go. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v7]
On Tue, 26 Apr 2022 17:27:35 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69
test/jdk/java/net/vthread/HttpALot.java line 76:
> 74: var address = server.getAddress();
> 75: URL url = new URL("http://"; + address.getHostName() + ":" +
> address.getPort() + "/hello");
> 76:
Nit: Ideally we should use the URIBuilder from the test library here, and the
IP literal address to improve stability of the test on machines that may have
strange /etc/hosts configuration. This can be taken care of after this PR has
been integrated.
test/jdk/java/net/vthread/InterruptHttp.java line 88:
> 86: InetAddress lb = InetAddress.getLoopbackAddress();
> 87: listener = new ServerSocket(0, -1, lb);
> 88: address = "http://"; + lb.getHostAddress() + ":" +
> listener.getLocalPort();
Same remark about using URIBuilder here. It would take care of properly
formatting the address in case of IPv6 literals. This can be taken care of
after this PR has been integrated.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v7]
On Tue, 26 Apr 2022 17:27:35 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69
src/jdk.management/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java
line 149:
> 147: * access to the file or {@link
> java.lang.management.ManagementPermission
> 148: * ManagementPermission("control")} is denied
> 149: * @since 19
Maybe there ought to be an `@throws UnsupportedOperationException` here since
that is what the default implementation of the method is supposed to do.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces
On Tue, 26 Apr 2022 22:24:26 GMT, Joe Darcy wrote: > To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. Hi Joe, just two suggestions about the javax.management changes. Otherwise looks good! src/java.management/share/classes/javax/management/openmbean/ArrayType.java line 96: > 94: * > 95: * @param the Java type that instances described by this type must > 96: * have. Instead of "instances" - would it be more correct to say "array elements"? src/java.management/share/classes/javax/management/openmbean/SimpleType.java line 60: > 58: * @param the Java type that instances described by this type must > 59: * have. > 60: * I would suggest to say "that values described by this type"... - PR: https://git.openjdk.java.net/jdk/pull/8410
Re: Integrated: 8285677: ProblemList two tests from JDK-8285671 on macosx-x64
On Tue, 26 Apr 2022 19:35:19 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList two tests from JDK-8285671 on macosx-x64. LGTM Dan. Thanks for taking care of that! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8406
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v7]
On Tue, 26 Apr 2022 17:27:35 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69
src/java.base/share/classes/sun/nio/ch/DatagramChannelImpl.java line 770:
> 768: synchronized (p) {
> 769: DatagramPackets.setLength(p, n);
> 770: p.setSocketAddress(sender);
Hmmm... For integrity it might be better to call the public
`DatagramPacket::setData(byte[] buf, int offset, int length)` method here now.
The additional advantage is that the private access to
`DatagramPackets.setLength(p, n);` will no longer be needed.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Integrated: 8284779: Test java/util/logging/Logger/logrb/TestLogrbResourceBundle.java fails intermittently with vthreads wrapper
On Mon, 25 Apr 2022 13:30:05 GMT, Daniel Fuchs wrote: > Hi, > > Please find enclosed a patch to fix a rare intermittent failure that was > detected while testing virtual threads. > The issue has nothing to do with virtual threads, the test is simply missing > a reachability fence to make sure that the parent logger is not garbage > collected until its child logger is created. > > best regards, > > -- daniel This pull request has now been integrated. Changeset: 552e1b0b Author:Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/552e1b0b8a0cd49089f58dea92ca96cce86b311f Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod 8284779: Test java/util/logging/Logger/logrb/TestLogrbResourceBundle.java fails intermittently with vthreads wrapper Reviewed-by: alanb - PR: https://git.openjdk.java.net/jdk/pull/8382
Re: RFR: 8284779: Test java/util/logging/Logger/logrb/TestLogrbResourceBundle.java fails intermittently with vthreads wrapper [v2]
On Mon, 25 Apr 2022 13:48:47 GMT, Alan Bateman wrote: >> Daniel Fuchs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update copyright year again > > test/jdk/java/util/logging/Logger/logrb/TestLogrbResourceBundle.java line 3: > >> 1: /* >> 2: * Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > I think you meant 2022 :-) How times fly! :-) Thanks for catching. - PR: https://git.openjdk.java.net/jdk/pull/8382
Re: RFR: 8284779: Test java/util/logging/Logger/logrb/TestLogrbResourceBundle.java fails intermittently with vthreads wrapper [v2]
> Hi, > > Please find enclosed a patch to fix a rare intermittent failure that was > detected while testing virtual threads. > The issue has nothing to do with virtual threads, the test is simply missing > a reachability fence to make sure that the parent logger is not garbage > collected until its child logger is created. > > best regards, > > -- daniel Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Update copyright year again - Changes: - all: https://git.openjdk.java.net/jdk/pull/8382/files - new: https://git.openjdk.java.net/jdk/pull/8382/files/0e7d60cb..191fa7ab Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8382&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8382&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8382.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8382/head:pull/8382 PR: https://git.openjdk.java.net/jdk/pull/8382
RFR: 8284779: Test java/util/logging/Logger/logrb/TestLogrbResourceBundle.java fails intermittently with vthreads wrapper
Hi, Please find enclosed a patch to fix a rare intermittent failure that was detected while testing virtual thread. The issue has nothing to do with virtual thread, the test is simply missing a reachability fence to make sure that the parent logger is not garbage collected until its child logger is created. best regards, -- daniel - Commit messages: - 8284779: Test java/util/logging/Logger/logrb/TestLogrbResourceBundle.java fails intermittently with vthreads wrapper Changes: https://git.openjdk.java.net/jdk/pull/8382/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8382&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284779 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8382.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8382/head:pull/8382 PR: https://git.openjdk.java.net/jdk/pull/8382
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v3]
On Fri, 22 Apr 2022 14:35:14 GMT, Sibabrata Sahoo wrote:
>> A new API to support replacing selective lines with desired content.
>
> Sibabrata Sahoo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update FileUtils.java
test/lib/jdk/test/lib/util/FileUtils.java line 402:
> 400: if (!removed.equals(froms)) {
> 401: throw new IOException("Removed not the same");
> 402: }
That's a bit strange. I would suggest to return the removed lines instead, or
to pass a `Consumer` (or even better, a `Predicate` ?) that
will accept the removed lines. You could continue to remove if the predicate
returns true and throw if it returns false. It would also enable you to tell
exactly which line failed the check.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285452: Support new API to replace a file content using FileUtils.java [v2]
On Fri, 22 Apr 2022 12:36:15 GMT, Sibabrata Sahoo wrote:
>> A new API to support replacing selective lines with desired content.
>
> Sibabrata Sahoo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update FileUtils.java
test/lib/jdk/test/lib/util/FileUtils.java line 381:
> 379:
> 380: public static void patch(Path path, int fromLine, int toLine, String
> to) throws IOException {
> 381: if(fromLine < 1 || toLine < 1) {
It would be good to add a proper API doc comment, especially regarding the
meaning of the parameters, and whether the line in question is
included/excluded. Also RuntimeException could be replaced with
IndexOutOfBoundsException.
-
PR: https://git.openjdk.java.net/jdk/pull/8360
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 14:03:39 GMT, Daniel Fuchs wrote: >> I ran `codespell` on modules owned by the serviceability team >> (`java.instrument java.management.rmi java.management jdk.attach >> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi >> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and >> accepted those changes where it indeed discovered real typos. >> >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > LGTM. I spotted one fix in a exception message. I don't expect that there > will be tests depending on that, but it might still be a good idea to run the > serviceability tests to double check. Although I guess the test would have > had the same typo and would have been fixed too were it the case :-) > @dfuch I have only updated files in `src`, so if the incorrect spelling is > tested, that test will now fail. I'm unfortunately not well versed in what > tests cover serviceability code. Can you suggest a suitable set of tests to > run? Folks from serviceability-dev will be more able to answer that - but I would suggest running tier2-tier3 as a precaution. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the serviceability team > (`java.instrument java.management.rmi java.management jdk.attach > jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi > jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted > those changes where it indeed discovered real typos. > > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. LGTM. I spotted one fix in a exception message. I don't expect that there will be tests depending on that, but it might still be a good idea to run the serviceability tests to double check. Although I guess the test would have had the same typo and would have been fixed too were it the case :-) - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner
On Wed, 20 Apr 2022 00:32:25 GMT, Brent Christian wrote:
> Please review this change to replace the finalizer in
> `AbstractLdapNamingEnumeration` with a Cleaner.
>
> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult
> res`, and `LdapClient enumClnt`) are moved to a static inner class . From
> there, the change is fairly mechanical.
>
> Details of note:
> 1. Some operations need to change the state values (the update() method is
> probably the most interesting).
> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read
> `homeCtx` from the superclass's `state`.
>
> The test case is based on a copy of
> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case
> might be possible, but this was done for expediency.
>
> The test only confirms that the new Cleaner use does not keep the object
> reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration`
> or `LdapBindingEnumeration`, though all are subclasses of
> `AbstractLdapNamingEnumeration`).
>
> Thanks.
I also agree with Roger's suggestions.
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 73:
> 71: public void run() {
> 72: if (enumClnt != null) {
> 73: enumClnt.clearSearchReply(res, homeCtx.reqCtls);
It's a bit strange to see that there is no guard here to verify that `homeCtx
!= null`, when line 76 implies that it might. My reading is that `homeCtxt` is
not supposed to be `null` when `enumClnt` is not `null`. That could be
explained in a comment, or alternatively asserted just before line 73 (`assert
homeCtx != null;`)
src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
line 83:
> 81: }
> 82:
> 83: private CleaningAction state;
I wonder if state should be final?
-
PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset [v2]
On Wed, 20 Apr 2022 15:33:26 GMT, Brian Burkhalter wrote: >> Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods >> of `java.io.FilterInputStream`. > > Brian Burkhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8284930: Also remove synchronized keyword from mark() and reset() of > InputStream Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284930: Re-examine FilterInputStream mark/reset
On Wed, 20 Apr 2022 11:56:20 GMT, Jaikiran Pai wrote: > As for the changes to `FilterInputStream`, would this change require a CSR? There might be a case where a subclass of `FilterInputStream` has overridden all other methods to add `synchronized` but has not overridden `mark` and `reset`, relying on the synchronization of the super class. I have skimmed over subclasses in the JDK, and have found no evidence of such behavior (though I have not looked exhaustively at all subclasses). Of course such subclasses could exist in the wild, but that would be both odd and fragile, since synchronization is not specified. In this particular case I don't believe that we would need a CSR to remove a synchronized keyword: whether a method is synchronized or not is not documented and not part of the spec: https://download.java.net/java/early_access/jdk19/docs/api/java.base/java/io/FilterInputStream.html#mark(int) - and `FilterInputStream` says nothing about synchronization. I agree with Alan that `InputStream` should be modified similarly. - PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 19:28:08 GMT, Stuart Marks wrote: > Reviewers for i18n, net, nio, and security, please review call site changes > in your areas. Thanks. Changes to `java.net.http` look good to me. I haven't spotted anything obviously wrong in the rest, but should defer to reviewers of these areas. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Tue, 12 Apr 2022 13:02:44 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/io/BufferedReader.java line 101:
>>
>>> 99: */
>>> 100: public BufferedReader(Reader in, int sz) {
>>> 101: Objects.requireNonNull(in);
>>
>> Not sure if that even matters - but there will be a slight change of
>> behaviour here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`.
>> Instead of synchronizing on `in`, the `BufferedReader` will synchronize on
>> `this`.
>> Now that I think of it - it probably does matter since even if
>> CAN_USE_INTERNAL_LOCK is true, untrusted subclasses of BufferedReader
>> calling this constructor might expect the locking to be performed on `in`?
>
>> Not sure if that even matters - but there will be a slight change of
>> behaviour here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`.
>> Instead of synchronizing on `in`, the `BufferedReader` will synchronize on
>> `this`.
>
> Good! We can change this so that depends on whether BufferedReader is
> extended and whether the given Reader is trusted. It's not clear if anyone
> could reliably depend on undocumented behavior like this but we have to be
> cautious at the same time.
Thanks - the same issue appears with `BufferedWriter`/`Writer`.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Tue, 12 Apr 2022 01:17:37 GMT, Andrew John Hughes wrote: > What's the status of this? The last comment reads as if this is good to go. I believe we're still waiting for Martin to reply to the last comment: > Maybe, you could also log a bug for a test addition and describe in it an > environment, and sort of a high-level scenario of how JDK-8275535 can be > reproduced. Otherwise yes - this would be good to go. - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Fri, 8 Apr 2022 13:43:39 GMT, Alan Bateman wrote:
> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
> JDK version to target.
>
> We will refresh this PR periodically to pick up changes and fixes from the
> loom repo.
>
> Most of the new mechanisms in the HotSpot VM are disabled by default and
> require running with `--enable-preview` to enable.
>
> The patch has support for x64 and aarch64 on the usual operating systems
> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for zero
> and some of the other ports. Additional ports can be contributed via PRs
> against the fibers branch in the loom repo.
>
> There are changes in many areas. To reduce notifications/mails, the labels
> have been trimmed down for now to hotspot, serviceability and core-libs.
> We'll add the complete set of labels when the PR is further along.
>
> The changes include a refresh of java.util.concurrent and ForkJoinPool from
> Doug Lea's CVS. These changes will probably be proposed and integrated in
> advance of this PR.
>
> The changes include some non-exposed and low-level infrastructure to support
> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
> make life a bit easier and avoid having to separate VM changes and juggle
> branches at this time.
src/java.base/share/classes/java/io/BufferedReader.java line 101:
> 99: */
> 100: public BufferedReader(Reader in, int sz) {
> 101: Objects.requireNonNull(in);
Not sure if that even matters - but there will be a slight change of behaviour
here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`. Instead of
synchronizing on `in`, the `BufferedReader` will synchronize on `this`.
Now that I think of it - it probably does matter since even if
CAN_USE_INTERNAL_LOCK is true, untrusted subclasses of BufferedReader calling
this constructor might expect the locking to be performed on `in`?
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Integrated: 8283719: java/util/logging/CheckZombieLockTest.java failing intermittently
On Fri, 8 Apr 2022 16:37:07 GMT, Daniel Fuchs wrote:
> Please find enclosed a patch for
> `8283719: java/util/logging/CheckZombieLockTest.java failing intermittently`
>
> My analysis is that the test fails intermittently because the `FileChannel`
> created by the test is garbage collected too early, which releases the
> associated lock before the `FileHandler` is created.
> I have replaced the `try { } finally { }` with a `try-with-resource( ) { }
> finally { }` which will prevent the `FileChannel` from being released before
> the end of the block. Additional bonus: this should also help with the code
> that tries to cleanup the files at the end.
>
> Though I haven't been able to reproduce the exact failure yet, I haven't
> observed the new version of the test failing either.
This pull request has now been integrated.
Changeset: 74835f73
Author:Daniel Fuchs
URL:
https://git.openjdk.java.net/jdk/commit/74835f73893976c162ef5a441f0cfec16eb8706f
Stats: 16 lines in 1 file changed: 3 ins; 6 del; 7 mod
8283719: java/util/logging/CheckZombieLockTest.java failing intermittently
Reviewed-by: alanb
-
PR: https://git.openjdk.java.net/jdk/pull/8168
Re: RFR: 8283719: java/util/logging/CheckZombieLockTest.java failing intermittently [v2]
On Fri, 8 Apr 2022 17:09:42 GMT, Alan Bateman wrote: > Looks fine, you can drop use of getAbsolutePath if you want to, not important > here. Done - used `File::toPath` since `lock` is a `File`. - PR: https://git.openjdk.java.net/jdk/pull/8168
Re: RFR: 8283719: java/util/logging/CheckZombieLockTest.java failing intermittently [v3]
> Please find enclosed a patch for
> `8283719: java/util/logging/CheckZombieLockTest.java failing intermittently`
>
> My analysis is that the test fails intermittently because the `FileChannel`
> created by the test is garbage collected too early, which releases the
> associated lock before the `FileHandler` is created.
> I have replaced the `try { } finally { }` with a `try-with-resource( ) { }
> finally { }` which will prevent the `FileChannel` from being released before
> the end of the block. Additional bonus: this should also help with the code
> that tries to cleanup the files at the end.
>
> Though I haven't been able to reproduce the exact failure yet, I haven't
> observed the new version of the test failing either.
Daniel Fuchs has updated the pull request incrementally with one additional
commit since the last revision:
Incorporated review comments
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8168/files
- new: https://git.openjdk.java.net/jdk/pull/8168/files/46527e37..f0872022
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8168&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8168&range=01-02
Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/8168.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8168/head:pull/8168
PR: https://git.openjdk.java.net/jdk/pull/8168
Re: RFR: 8283719: java/util/logging/CheckZombieLockTest.java failing intermittently [v2]
> Please find enclosed a patch for
> `8283719: java/util/logging/CheckZombieLockTest.java failing intermittently`
>
> My analysis is that the test fails intermittently because the `FileChannel`
> created by the test is garbage collected too early, which releases the
> associated lock before the `FileHandler` is created.
> I have replaced the `try { } finally { }` with a `try-with-resource( ) { }
> finally { }` which will prevent the `FileChannel` from being released before
> the end of the block. Additional bonus: this should also help with the code
> that tries to cleanup the files at the end.
>
> Though I haven't been able to reproduce the exact failure yet, I haven't
> observed the new version of the test failing either.
Daniel Fuchs has updated the pull request incrementally with one additional
commit since the last revision:
Incorporated review comments
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8168/files
- new: https://git.openjdk.java.net/jdk/pull/8168/files/752d4ba0..46527e37
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8168&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8168&range=00-01
Stats: 12 lines in 1 file changed: 2 ins; 6 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/8168.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8168/head:pull/8168
PR: https://git.openjdk.java.net/jdk/pull/8168
Re: RFR: 8283719: java/util/logging/CheckZombieLockTest.java failing intermittently [v2]
On Fri, 8 Apr 2022 16:50:45 GMT, Alan Bateman wrote:
>> Daniel Fuchs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Incorporated review comments
>
> test/jdk/java/util/logging/CheckZombieLockTest.java line 247:
>
>> 245: try (FileChannel fc =
>> FileChannel.open(Paths.get(lock.getAbsolutePath()),
>> 246: StandardOpenOption.CREATE_NEW,
>> StandardOpenOption.APPEND,
>> 247: StandardOpenOption.WRITE)) {
>
> Changing this to use try-with-resources looks good. In passing I wonder why
> it calls lock.getAbsolutePath(), it's okay to call open with a relative path.
> Also, if you change this test to use import static then you could change this
> line to:
>
> `try (FileChannel fc = FileChannel.open(Path.of(lock), CREATE_NEW, APPEND,
> WRITE))`
Good idea. Done.
-
PR: https://git.openjdk.java.net/jdk/pull/8168
RFR: 8283719: java/util/logging/CheckZombieLockTest.java failing intermittently
Please find enclosed a patch for
`8283719: java/util/logging/CheckZombieLockTest.java failing intermittently`
My analysis is that the test fails intermittently because the `FileChannel`
created by the test is garbage collected too early, which releases the
associated lock before the `FileHandler` is created.
I have replaced the `try { } finally { }` with a `try-with-resource( ) { }
finally { }` which will prevent the `FileChannel` from being released before
the end of the block. Additional bonus: this should also help with the code
that tries to cleanup the files at the end.
Though I haven't been able to reproduce the exact failure yet, I haven't
observed the new version of the test failing either.
-
Commit messages:
- 8283719: java/util/logging/CheckZombieLockTest.java failing intermittently
Changes: https://git.openjdk.java.net/jdk/pull/8168/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8168&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8283719
Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/8168.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8168/head:pull/8168
PR: https://git.openjdk.java.net/jdk/pull/8168
Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Nice update to the test! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final
On Tue, 29 Mar 2022 01:07:33 GMT, Stuart Marks wrote: > Pretty much just what it says. Yes - it looks like jdk/internal/reflect/CallerSensitive/CheckCSMs.java needs to be updated. - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283349 : Robustness improvements to java/util/prefs/AddNodeChangeListener.jar
On Tue, 22 Mar 2022 17:59:15 GMT, Brent Christian wrote: > Please review this change to the java/util/prefs/AddNodeChangeListener.jar > test. > > Although the test specifies "-Djava.util.prefs.userRoot=." to ensure a fresh > Preferences on each test run, MacOS does not seem to honor this, and still > stores prefs in Library/. > > This test has long suffered intermittent failures. 8255031 added some > debugging code; as of that change, the test fails fast as soon as an expected > change event is not received. In particular, if the expected event is not > received for adding the node, the test exits without removing the node. In > this way, on Mac, the node can get "stuck" in the preferences of the machine. > Future runs on the machine will already have the node, no node added change > event will be generated (because the node was already there), the test will > continue to fail without removing the node, etc. This matches observations on > some CI machines. > > This change ensures that: > * the test node is not present before the test > * the test runs to completion, including removing the test node > * the test node is not left behind after the test > > Thanks. I'm not a regular contributor in this area, so maybe wait to see if a second opinion is forthcoming, but otherwise the proposed changes look reasonable to me. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7908
Re: RFR: JDK-8283480: Make AbstractStringBuilder sealed
On Tue, 22 Mar 2022 00:01:59 GMT, Joe Darcy wrote: > As part of updating the core libraries to be sealed, the package-access > AbstractStringBuilder, implementation superclass of StringBuilder and > StringBuffer, can be marked as sealed with those two subclasses on its > permits list. LGTM. The two subclasses are final. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7898
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v11]
On Mon, 21 Mar 2022 14:24:30 GMT, Jim Laskey wrote:
>> We propose to provide a runtime anonymous carrier class object generator;
>> java.lang.runtime.Carrier. This generator class is designed to share
>> anonymous classes when shapes are similar. For example, if several clients
>> require objects containing two integer fields, then Carrier will ensure that
>> each client generates carrier objects using the same underlying anonymous
>> class.
>>
>> See JBS for details.
>
> Jim Laskey 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 18 additional commits since
> the last revision:
>
> - Remove LOOKUP static final
> - Merge branch 'master' into 8282798
> - Typos
> - Update Carrier.java
> - Redo API to use list, bring Carrier.component back
> - Clean up API
> - Remove redundant MethodHandle component(MethodType methodType, int i) API
> - Revert to {@return} style comments.
> - Clarify primitive store in array carriers.
> - Use long array for primitives
> - ... and 8 more:
> https://git.openjdk.java.net/jdk/compare/eace55cb...a8657bbe
src/java.base/share/classes/java/lang/runtime/Carrier.java line 166:
> 164: from = longIndex++;
> 165: filter = DOUBLE_TO_LONG;
> 166: } else if(ptype == float.class) {
Trivial: space missing after `if`
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8283049: Fix non-singleton LoggerFinder error message: s/on/one [v2]
On Fri, 11 Mar 2022 19:18:55 GMT, Carter Kozak wrote: >> 8283049: Fix non-singleton LoggerFinder error message: s/on/one > > Carter Kozak has updated the pull request incrementally with one additional > commit since the last revision: > > Add 8283049 to LoggerFinderLoaderTest @bug docs Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7780
Re: RFR: 8283049: Fix non-singleton LoggerFinder error message: s/on/one
On Thu, 10 Mar 2022 22:28:14 GMT, Carter Kozak wrote:
> 8283049: Fix non-singleton LoggerFinder error message: s/on/one
I changed the bug title to match the PR title.
test/jdk/java/lang/System/LoggerFinder/internal/LoggerFinderLoaderTest/LoggerFinderLoaderTest.java
line 233:
> 231: }
> 232: } else if (singleton) {
> 233: if
> (!warning.contains("java.util.ServiceConfigurationError: More than one
> LoggerFinder implementation")) {
please add 8283049 to the `@bug` list above since the test actually verifies
the fix.
-
PR: https://git.openjdk.java.net/jdk/pull/7780
Re: Request for an issue for LoggerFinderLoader error-message typo
Hi Carter, I have created https://bugs.openjdk.java.net/browse/JDK-8283049 I will sponsor the changed after it's been reviewed and integrated. best regards, -- daniel On 11/03/2022 15:42, Carter Kozak wrote: When multiple LoggerFinder implementations are found, a ServiceConfigurationError is thrown with message "More than on LoggerFinder implementation" rather than "More than one LoggerFinder implementation" (s/on/one).
Re: RFR: 8280400: JDK 19 L10n resource files update - msgdrop 10
On Thu, 10 Mar 2022 17:00:09 GMT, Naoto Sato wrote: > IIRC, localized resource files should have the same copyright year as the > base English one. That's what I was told by the l10n engineer when I had the > same comment. Thanks Naoto! I have no objection then. - PR: https://git.openjdk.java.net/jdk/pull/7765
Re: RFR: 8280400: JDK 19 L10n resource files update - msgdrop 10
On Wed, 9 Mar 2022 21:09:30 GMT, Alisen Chung wrote: > msg drop for jdk19, Mar 9, 2022 For simple webserver resource files - should the copyright year be 2022? - PR: https://git.openjdk.java.net/jdk/pull/7765
Re: RFR: 8282662: Use List/Set.of() factory methods to reduce memory consumption
On Mon, 7 Mar 2022 15:11:50 GMT, Сергей Цыпанов wrote: > `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with > smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when > called with vararg of size 0, 1, 2. > > In general replacement of `Arrays.asList()` with `List.of()` is dubious as > the latter is null-hostile, however in some cases we are sure that arguments > are non-null. Into this PR I've included the following cases (in addition to > those where the argument is proved to be non-null at compile-time): > - `MethodHandles.longestParameterList()` never returns null > - parameter types are never null > - interfaces used for proxy construction and returned from > `Class.getInterfaces()` are never null > - exceptions types of method signature are never null There's also another difference: they have different serial forms. From my cursory inspection it doesn't look like the returned list are put in serializable fields but it could be good to double check it. - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character [v3]
On Tue, 1 Mar 2022 08:13:57 GMT, Chris Hegarty wrote: >> The module finder implementation incorrectly uses the path-separator >> character from the default file system, when mapping the relative path >> of an entry in an exploded module to a package name. This causes >> problems on Windows [*] when using a module finder with a custom file >> system that has a path-separator character that differs from that of the >> default file system, e.g. the zip file system (which uses '/', >> rather than '\' ). >> >> Rather than adding a new test for this, I deciced to just modify an >> existing one. The existing test exercises the module finder with a >> custom file system, but does so with modules have trivial single-level >> packages names. I just expanded the module to have a subpackage. This >> is sufficient to reproduce the bug, and verify the fix. >> >> [*] >> >> java.lang.module.FindException: Error reading module: /m2 >> at >> java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) >> at >> java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) >> at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) >> at >> java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) >> at >> java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) >> at >> ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) >> at >> ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) >> at >> ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) >> at ... >> Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r >> not found in module >> at >> java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) >> at >> java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) >> at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) >> at >> java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) >> at >> java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) >> ... 36 more > > Chris Hegarty 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 four additional > commits since the last revision: > > - Merge branch 'openjdk:master' into modulefinder_separator_win > - update copyright year > - add tag with OrigBugId 8178380, and bugFixId 8282444 > - fix file separator in module finder with custom fs Not an expert of the area but the proposed changes look good to me. - PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282444: Module finder incorrectly assumes default file system path-separator character
On Mon, 28 Feb 2022 11:12:17 GMT, Chris Hegarty wrote: > The module finder implementation incorrectly uses the path-separator > character from the default file system, when mapping the relative path > of an entry in an exploded module to a package name. This causes > problems on Windows [*] when using a module finder with a custom file > system that has a path-separator character that differs from that of the > default file system, e.g. the zip file system (which uses '/', > rather than '\' ). > > Rather than adding a new test for this, I deciced to just modify an > existing one. The existing test exercises the module finder with a > custom file system, but does so with modules have trivial single-level > packages names. I just expanded the module to have a subpackage. This > is sufficient to reproduce the bug, and verify the fix. > > [*] > > java.lang.module.FindException: Error reading module: /m2 > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:350) > at > java.base/jdk.internal.module.ModulePath.scanDirectory(ModulePath.java:284) > at java.base/jdk.internal.module.ModulePath.scan(ModulePath.java:232) > at > java.base/jdk.internal.module.ModulePath.scanNextEntry(ModulePath.java:190) > at > java.base/jdk.internal.module.ModulePath.findAll(ModulePath.java:166) > at > ModulesInCustomFileSystem.listAllModules(ModulesInCustomFileSystem.java:108) > at > ModulesInCustomFileSystem.testZipFileSystem(ModulesInCustomFileSystem.java:97) > at > ModulesInCustomFileSystem.testExplodedModulesInZipFileSystem(ModulesInCustomFileSystem.java:68) > at ... > Caused by: java.lang.module.InvalidModuleDescriptorException: Package q.r not > found in module > at > java.base/jdk.internal.module.ModuleInfo.invalidModuleDescriptor(ModuleInfo.java:1212) > at > java.base/jdk.internal.module.ModuleInfo.doRead(ModuleInfo.java:330) > at java.base/jdk.internal.module.ModuleInfo.read(ModuleInfo.java:129) > at > java.base/jdk.internal.module.ModulePath.readExplodedModule(ModulePath.java:689) > at > java.base/jdk.internal.module.ModulePath.readModule(ModulePath.java:320) > ... 36 more Hi Chris, maybe you should add `@bug 8282444` to `ModulesInCustomFileSystem.java` too? - PR: https://git.openjdk.java.net/jdk/pull/7632
Re: RFR: 8282190: Typo in javadoc of java.time.format.DateTimeFormatter#getDecimalStyle
On Mon, 21 Feb 2022 12:42:37 GMT, Jaikiran Pai wrote: > Can I please get a review of this trivial change to the javadoc of > `DateTimeFormatter.getDecimalStyle()` method which fixes the typo noted in > https://bugs.openjdk.java.net/browse/JDK-8282190? LGTM - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7556
Re: RFR: 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters
On Mon, 14 Feb 2022 05:27:39 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8281634? > > The commit introduces the missing `err.invalid.filters` key in the jdeps > resource bundle. I have run a simple check to make sure this resource bundle > doesn't miss any other `err.` keys. From a simple search, following are the > unique `err.` keys used in the code of jdeps (under > `src/jdk.jdeps/share/classes/com/sun/tools/jdeps` directory): > > err.exception.message > err.invalid.options > err.multirelease.version.associated > err.missing.arg > err.multirelease.jar.malformed > err.option.already.specified > err.missing.dependences > err.module.not.found > err.invalid.path > err.genmoduleinfo.not.jarfile > err.invalid.arg.for.option > err.multirelease.option.notfound > err.filter.not.specified > err.unknown.option > err.command.set > err.invalid.filters > err.genmoduleinfo.unnamed.package > err.option.after.class > > Apart from the `err.invalid.filters` key which this PR is fixing, none of the > other keys are missing from the resource bundle. I haven't updated the > resource bundles for Japanese and Chinese languages because I don't know > their equivalent values and looking at the commit history of those files, it > appears that those changes are done as separate tasks (should a JBS issue be > raised for this?) > > The existing `test/langtools/tools/jdeps/Options.java` jtreg test has been > updated to reproduce the issue and verify this fix. > > An important detail about the update to the test is that while working on the > update to this test, I realized that the current implementation in the test > isn't fully functional. As a result, this test is currently, incorrectly > considered as passing. Specifically, the test was missing a `assertTrue` call > against the ouput/error content generated by the run of the `jdeps` tool. > This PR adds that assertion. > Once that assertion is added, it started showing up 3 genuine failures. These > failures are within that test code: > - The test uses `-jdkinternal` instead of `-jdkinternals`. This appears to be > a typo when this section in the test was added. The commit history of the > source of jdeps tool shows that this option was always `-jdkinternals`. This > PR fixes that part in the test. > - The test expects an error message "-R cannot be used with --inverse option" > when `-R` and `--inverse` are used together. However, at some point the > source of jdeps tool was changed (as part of > https://bugs.openjdk.java.net/browse/JDK-8213909) to return a different error > message. That changes appears to have missed changing this test case error > message and since this test has been falsely passing, it never got caught. > This PR now fixes that issue by expecting the correct error message. > - The test was expecting an error message "--list-deps and > --list-reduced-deps options are specified" when "--list-deps" was used along > with "--summary". This appears to be a copy/paste error in the test case and > wasn't caught because the test was falsely passing. This too has been fixed > in this PR. > > With the fixes in this test, the test now reproduces the original issue and > verifies the fix. I realize that this PR has changes in the test that aren't > directly related to the issue raised in > https://bugs.openjdk.java.net/browse/JDK-8281634, but those changes are > necessary to get the test functional. However if a separate JBS issue needs > to be opened to track those changes, please do let me know and I'll address > these test changes in a separate PR. The changes look reasonable. Thanks for taking that on Jaikiran! Maybe wait one day or two before integrating to give Mandy a chance to chime in. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7455
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Thu, 10 Feb 2022 15:25:09 GMT, Aleksei Efimov wrote: > I'm ok with pushing the fix without a test given that the user will verify it > in his real environment. > Maybe, you could also log a bug for a test addition and describe in it an > environment, and sort of a high-level scenario of how JDK-8275535 can be > reproduced. Right - I would be fine with that too. - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to > the state previous to JDK-8160768, where an authentication failure stops from > trying other LDAP servers with the same credentials [1]. After JDK-8160768 we > have 2 possible loops to stop: the one that iterates over different URLs and > the one that iterates over different endpoints (after a DNS query that > returns multiple values). > > No test regressions observed in jdk/com/sun/jndi/ldap. > > -- > [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137 The concerns are two folds: - without a regression test, you have to trust that the source changes actually work, and there's typically no verification possible - without a regression test, the next refactoring change in this area two years from now could undo the fix and nobody would notice I agree that the changes seem safe and given the history seem to be doing what the fix/PR says they do, so I'd be more concerned with the latter. So if it's practical to add a test I'd rather have one. If the behavior is too hard to test - then the appropriate keyword would be `noreg-hard` rather than `noreg-trivial` (I am not sure trivial actually qualifies here - I can see no obvious flaw but I'm not sure we can say that there are obviously no flaws - or that a flaw would become obvious to future maintainers if that code was refactored in a way that removed the fix). - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to > the state previous to JDK-8160768, where an authentication failure stops from > trying other LDAP servers with the same credentials [1]. After JDK-8160768 we > have 2 possible loops to stop: the one that iterates over different URLs and > the one that iterates over different endpoints (after a DNS query that > returns multiple values). > > No test regressions observed in jdk/com/sun/jndi/ldap. > > -- > [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137 Is the new behavior something that could be tested with a new non regression test? See existing tests in `test/jdk/com/sun/jndi/ldap` - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: Seeking inputs on 8224794: ZipFile/JarFile should open zip/JAR file with FILE_SHARE_DELETE sharing mode on Windows
Hi Jaikiran, Thanks for working on this and apologies for the long silence. I believe your analysis of the issue is very valuable. Unless there is some clever trick we could do to allow to unlink the file from the file system before deleting it, so that the file path can be reused, it seems indeed that using FILE_SHARE_DELETE doesn't buy us much benefit. It is a pity, but IMO it was well worth the investigation. Unless others on this list disagree, or can suggest other tricks, I would suggest shelving this work for now. best regards, -- daniel On 18/12/2021 06:00, Jaikiran Pai wrote: Would there be interest in moving this forward? Enabling that FILE_SHARE_DELETE option has opened up some questions that I noted in my previous mail below. So in order to move forward I would need some inputs. If we don't want to do this at this time, I'll close the draft PR that's currently open https://github.com/openjdk/jdk/pull/6477. -Jaikiran
Re: RFR: 8272996: JNDI DNS provider fails to resolve SRV entries when IPV6 stack is enabled
On Fri, 4 Feb 2022 15:36:35 GMT, Aleksei Efimov wrote: > Hi, > > JNDI's `DnsClient` can fail with `UncheckedIOException` during `connect` or > `disconnect` method calls. It is a [know > behavior](https://bugs.openjdk.java.net/browse/JDK-8236076) of > `DatagramSocket`. > > Currently, `DnsClient` method is fast-fails when `UncheckedIOException` is > observed during connect attempt. But it supposed to mark such server as > invalid and continue checking other servers. > > Testing: tier1, tier2, tier3 and existing JCK tests show no failures relevant > to the proposed changed LGTM - and a good cleanup to boot! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7353
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v13]
On Fri, 28 Jan 2022 16:58:55 GMT, Michael McMahon wrote: >> Hi, >> >> This change adds Channel Binding Token (CBT) support to HTTPS >> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, >> Kerberos) authentication scheme. When enabled, the implementation >> preemptively includes a CBT with authentication requests over Kerberos. The >> feature is enabled as follows: >> >> A system property "jdk.spnego.cbt" is defined which can have the values >> "never" (default), which means the feature is disabled, "always", which >> means the CBT is included for all https Negotiate authentications, or it can >> take the form "domain:a,b.c,*.d.com" which is a comma separated list of >> domains/hosts where the feature is enabled, and disabled everywhere else. In >> the given example, the CBT would be included in authentication requests for >> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its >> sub-domains. >> >> A test will be added separately to the implementation. >> >> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842 >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > spec update from CSR Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: JarIndex not following the specification
Hi Tobias, Regardless of whether this is a bug or not, JarIndex has been disabled by default in JDK 18, in the hope to eventually remove it in the future. See https://bugs.openjdk.java.net/browse/JDK-8273473 best regards, -- daniel On 27/01/2022 18:41, Tobias Stadler wrote: Hello everyone, According to https://docs.oracle.com/en/java/javase/17/docs/specs/jar/jar.html, sections in META-INF/INDEX.LIST are divided by blank lines. The write method of jdk.internal.util.jar.JarIndex does write those blank lines. But the read method begins a new section whenever it sees a line that ends with .jar. Does anybody know why JarIndex#read does not follow the specification? Is this a bug? E.g. some.jar META-INF META-INF/another.jar org Will be be read as two sections instead of just one. Also the package org will be mapped to META-INF/another.jar instead of some.jar. Best reagrds Tobias
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v12]
On Thu, 27 Jan 2022 18:05:25 GMT, Michael McMahon wrote: >> Hi, >> >> This change adds Channel Binding Token (CBT) support to HTTPS >> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, >> Kerberos) authentication scheme. When enabled, the implementation >> preemptively includes a CBT with authentication requests over Kerberos. The >> feature is enabled as follows: >> >> A system property "jdk.spnego.cbt" is defined which can have the values >> "never" (default), which means the feature is disabled, "always", which >> means the CBT is included for all https Negotiate authentications, or it can >> take the form "domain:a,b.c,*.d.com" which is a comma separated list of >> domains/hosts where the feature is enabled, and disabled everywhere else. In >> the given example, the CBT would be included in authentication requests for >> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its >> sub-domains. >> >> A test will be added separately to the implementation. >> >> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842 >> >> Thanks, >> Michael > > Michael McMahon 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: > > - test update > - Merge branch 'master' into spnego > - test update > - removed ^M from test > - Added unit test and comment update > - final review update (pre CSR) > - more updates > - fixed failing test issue and update for latest comments > - Merge branch 'master' into spnego > - added root cause to NamingException > - ... and 5 more: > https://git.openjdk.java.net/jdk/compare/ceb44101...59f703da LGTM! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
On Wed, 26 Jan 2022 19:00:14 GMT, Weijun Wang wrote:
>> test/jdk/sun/security/krb5/auto/HttpsCB.java line 201:
>>
>>> 199: return reader.readLine().equals(CONTENT);
>>> 200: } catch (Exception e) {
>>> 201: return false;
>>
>> Should we log that we have received the excepted exception here? Shouldn't
>> the catch clause only list the exceptions that we are expecting?
>
> It's `java.net.SocketException: Unexpected end of file from server`. Does not
> include any CBT words so don't know if it's worth parsing.
Thanks. Then it would be better to catch only `SocketException` here rather
than `Exception`.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
On Wed, 26 Jan 2022 16:02:09 GMT, Michael McMahon wrote:
>> Hi,
>>
>> This change adds Channel Binding Token (CBT) support to HTTPS
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO,
>> Kerberos) authentication scheme. When enabled, the implementation
>> preemptively includes a CBT with authentication requests over Kerberos. The
>> feature is enabled as follows:
>>
>> A system property "jdk.spnego.cbt" is defined which can have the values
>> "never" (default), which means the feature is disabled, "always", which
>> means the CBT is included for all https Negotiate authentications, or it can
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of
>> domains/hosts where the feature is enabled, and disabled everywhere else. In
>> the given example, the CBT would be included in authentication requests for
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
>> sub-domains.
>>
>> A test will be added separately to the implementation.
>>
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> removed ^M from test
Looks mostly good. Some doubts about catching just any exception
indiscriminately though.
test/jdk/sun/security/krb5/auto/HttpsCB.java line 120:
> 118:
> 119: boolean expected1 = Boolean.parseBoolean(args[0]);
> 120: boolean expected2 = Boolean.parseBoolean(args[1]);
It might be better for future maintainers and readability if these two
variables could have better names, and possibly a comment to explain their
purpose. AFAIU it's the expected result of running with/without CBT - where
`true` means that the operation should succeed and `false` that it's expected
to fail with some exception...
test/jdk/sun/security/krb5/auto/HttpsCB.java line 201:
> 199: return reader.readLine().equals(CONTENT);
> 200: } catch (Exception e) {
> 201: return false;
Should we log that we have received the excepted exception here? Shouldn't the
catch clause only list the exceptions that we are expecting?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v8]
On Tue, 25 Jan 2022 10:30:20 GMT, Michael McMahon wrote: >> Hi, >> >> This change adds Channel Binding Token (CBT) support to HTTPS >> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, >> Kerberos) authentication scheme. When enabled, the implementation >> preemptively includes a CBT with authentication requests over Kerberos. The >> feature is enabled as follows: >> >> A system property "jdk.spnego.cbt" is defined which can have the values >> "never" (default), which means the feature is disabled, "always", which >> means the CBT is included for all https Negotiate authentications, or it can >> take the form "domain:a,b.c,*.d.com" which is a comma separated list of >> domains/hosts where the feature is enabled, and disabled everywhere else. In >> the given example, the CBT would be included in authentication requests for >> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its >> sub-domains. >> >> A test will be added separately to the implementation. >> >> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842 >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > final review update (pre CSR) The new version LGTM. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
On Mon, 24 Jan 2022 13:36:47 GMT, Michael McMahon wrote:
>> Hi,
>>
>> This change adds Channel Binding Token (CBT) support to HTTPS
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO,
>> Kerberos) authentication scheme. When enabled, the implementation
>> preemptively includes a CBT with authentication requests over Kerberos. The
>> feature is enabled as follows:
>>
>> A system property "jdk.spnego.cbt" is defined which can have the values
>> "never" (default), which means the feature is disabled, "always", which
>> means the CBT is included for all https Negotiate authentications, or it can
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of
>> domains/hosts where the feature is enabled, and disabled everywhere else. In
>> the given example, the CBT would be included in authentication requests for
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
>> sub-domains.
>>
>> A test will be added separately to the implementation.
>>
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>>
>> Thanks,
>> Michael
>
> Michael McMahon 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 eight additional
> commits since the last revision:
>
> - fixed failing test issue and update for latest comments
> - Merge branch 'master' into spnego
> - added root cause to NamingException
> - more tidy-up
> - removed sasl module dependency and added SaslException cause
> - changes after first review round
> - cleanup but still no test. Will be added in closed repo
> - First version of fix. No test and feature enabled always.
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 260:
> 258: * @throws ChannelBindingException
> 259: */
> 260: private static TlsChannelBindingType parseType(String cbType) throws
> ChannelBindingException {
Maybe this method could throw NamingException directly now? That would avoid
wrapping CBE into NamingException?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: JDK-8280492: Address remaining doclint issues in JDK build
On Sat, 22 Jan 2022 21:09:03 GMT, Joe Darcy wrote:
> Use presumed syntax that will be introduced by JDK-8280488.
Marked as reviewed by dfuchs (Reviewer).
LGTM. I hope in the future IDEs will pick that rule up and offer some help when
writing `{@link }` `@see`...
-
PR: https://git.openjdk.java.net/jdk/pull/7189
Re: RFR: 8280470: Confusing instanceof check in HijrahChronology.range
On Mon, 17 Jan 2022 21:02:35 GMT, Andrey Turbanov wrote: > Parameter `ChronoField field` is checked by `if (field instanceof > ChronoField)`. Such check is confusing, because only one case, when this > could be `false` is when `field == null`. > But if condition is not satisfied we will get immediate NullPointerException > anyway in `return field.range();`. Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7118
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]
On Fri, 21 Jan 2022 16:02:29 GMT, Michael McMahon wrote: >> Hi, >> >> This change adds Channel Binding Token (CBT) support to HTTPS >> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, >> Kerberos) authentication scheme. When enabled, the implementation >> preemptively includes a CBT with authentication requests over Kerberos. The >> feature is enabled as follows: >> >> A system property "jdk.spnego.cbt" is defined which can have the values >> "never" (default), which means the feature is disabled, "always", which >> means the CBT is included for all https Negotiate authentications, or it can >> take the form "domain:a,b.c,*.d.com" which is a comma separated list of >> domains/hosts where the feature is enabled, and disabled everywhere else. In >> the given example, the CBT would be included in authentication requests for >> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its >> sub-domains. >> >> A test will be added separately to the implementation. >> >> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842 >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > added root cause to NamingException Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v4]
On Fri, 21 Jan 2022 13:06:40 GMT, Rob McKenna wrote: >> This fix attemps to resolve an issue where threads can stack up on each >> other while waiting to get a connection from the ldap pool to an unreachable >> server. It does this by having each thread start a countdown prior to >> holding the pools' lock. (which has been changed to a ReentrantLock) Once >> the lock has been grabbed, the timeout is adjusted to take the waiting time >> into account and the process of getting a connection from the pool or >> creating a new one commences. >> >> Note: this fix also changes the meaning of the connection pools initSize >> somewhat. In a situation where we have a large initSize and a small timeout >> the first thread could actually exhaust the timeout before creating all of >> its initial connections. Instead this fix simply creates a single connection >> per pool-connection-request. It continues to do so for subsequent requests >> regardless of whether an existing unused connection is available in the pool >> until initSize is exhausted. As such it may require a CSR. > > Rob McKenna has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains seven commits: > > - Add method to guard long to int cast > - Allow the test to pass on MacOSX > - JDK-8277795: whitespace > - Add test > - JDK-8277795: formatting > - JDK-8277795: whitespace cleanup > - JDK-8277795: ldap connection timeout not honoured under contention Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6568
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v4]
On Fri, 21 Jan 2022 15:26:33 GMT, Michael McMahon wrote:
>> Hi,
>>
>> This change adds Channel Binding Token (CBT) support to HTTPS
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO,
>> Kerberos) authentication scheme. When enabled, the implementation
>> preemptively includes a CBT with authentication requests over Kerberos. The
>> feature is enabled as follows:
>>
>> A system property "jdk.spnego.cbt" is defined which can have the values
>> "never" (default), which means the feature is disabled, "always", which
>> means the CBT is included for all https Negotiate authentications, or it can
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of
>> domains/hosts where the feature is enabled, and disabled everywhere else. In
>> the given example, the CBT would be included in authentication requests for
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
>> sub-domains.
>>
>> A test will be added separately to the implementation.
>>
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> more tidy-up
Marked as reviewed by dfuchs (Reviewer).
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 144:
> 142: } catch (ChannelBindingException e) {
> 143: throw new NamingException(e.getMessage());
> 144: }
Should we call initCause here and above? I see that we do call initCause in
NegotiatorImpl.java.
On the one hand it's better for diagnostic. On the other hand it exposes a
module-internal exception class which is not great. Or maybe we should set the
cause of the CBE as the cause of NamingException.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v4]
On Fri, 21 Jan 2022 15:26:33 GMT, Michael McMahon wrote:
>> Hi,
>>
>> This change adds Channel Binding Token (CBT) support to HTTPS
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO,
>> Kerberos) authentication scheme. When enabled, the implementation
>> preemptively includes a CBT with authentication requests over Kerberos. The
>> feature is enabled as follows:
>>
>> A system property "jdk.spnego.cbt" is defined which can have the values
>> "never" (default), which means the feature is disabled, "always", which
>> means the CBT is included for all https Negotiate authentications, or it can
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of
>> domains/hosts where the feature is enabled, and disabled everywhere else. In
>> the given example, the CBT would be included in authentication requests for
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
>> sub-domains.
>>
>> A test will be added separately to the implementation.
>>
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> more tidy-up
src/java.base/share/classes/sun/net/www/http/HttpClient.java line 191:
> 189: } else {
> 190: logError("Unexpected value for \"jdk.https.negotiate.cbt\"
> system property");
> 191: return "never";
Much easier to review now :-)
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Fri, 21 Jan 2022 15:08:58 GMT, Michael McMahon wrote: >> It was being handled elsewhere as "never". But, I agree it would be clearer >> to normalise it to "never" here. > > Sorry, I should have checked back to the source rather than the snippet > quoted. The problem is that the logError call is in the wrong place. It > should be before line 186. Though some other adjustments are also required Ah! Yes - I was also bitten again by the negation in the `if` too. The presence of `logError` in the body of the `if` will make it clearer :-) - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Thu, 20 Jan 2022 10:58:27 GMT, Michael McMahon wrote:
>> Hi,
>>
>> This change adds Channel Binding Token (CBT) support to HTTPS
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO,
>> Kerberos) authentication scheme. When enabled, the implementation
>> preemptively includes a CBT with authentication requests over Kerberos. The
>> feature is enabled as follows:
>>
>> A system property "jdk.spnego.cbt" is defined which can have the values
>> "never" (default), which means the feature is disabled, "always", which
>> means the CBT is included for all https Negotiate authentications, or it can
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of
>> domains/hosts where the feature is enabled, and disabled everywhere else. In
>> the given example, the CBT would be included in authentication requests for
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
>> sub-domains.
>>
>> A test will be added separately to the implementation.
>>
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> removed sasl module dependency and added SaslException cause
src/java.base/share/classes/java/net/doc-files/net-properties.html line 220:
> 218: This controls the generation and sending of TLS channel binding tokens
> (CBT) when Kerberos
> 219: or the Negotiate authentication scheme using Kerberos are
> employed over HTTPS with
> 220: {@code HttpURLConnection}. There are three possible settings:
Should it be `{@code HttpsURLConnection}`?
(BTW - can we use {@code } here ? Would be worth checking the generated doc)
src/java.base/share/classes/sun/net/www/http/HttpClient.java line 189:
> 187: } else {
> 188: logError("Unexpected value for \"jdk.https.negotiate.cbt\"
> system property");
> 189: return s;
Should this return either "always" or "never" instead? It seems that junk
values will be treated as "always". It would be better to make it clear here.
src/java.base/share/classes/sun/security/util/ChannelBindingException.java line
31:
> 29: * Thrown by TlsChannelBinding if an error occurs
> 30: */
> 31: public class ChannelBindingException extends Exception {
Should this extend `GeneralSecurityException` instead? Or should we just remove
this class and throw plain `GeneralSecurityException` in `TlsChannelBinding` ?
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 143:
> 141: tlsCB = TlsChannelBinding.create(cert);
> 142: } catch (ChannelBindingException e) {
> 143: throw new SaslException(e.getMessage());
Why is there a difference compared to line 133?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v3]
On Wed, 19 Jan 2022 15:13:57 GMT, Rob McKenna wrote: >> src/java.naming/share/classes/com/sun/jndi/ldap/pool/PooledConnectionFactory.java >> line 54: >> >>> 52: * @param timeout the connection timeout >>> 53: */ >>> 54: public abstract PooledConnection >>> createPooledConnection(PoolCallback pcb, long timeout) >> >> why not use int timeout to be consistent with existing code ? >> You've been required to "squash" it into an int in the factory ? > > IIRC this was a request from an earlier review. (long being the standard > throughout other new public apis) I'm happy with either, but int does avoid > the trouble of casting. Well I guess the request was "why not use long everywhere to avoid casting to int" ;-) But I'm happy with either too - as long as the place where you have a long (e.g obtained by substracting two nano times) and call a method that takes an int has the proper guards in place, and either assert/throws/floor or ceil if the assumptions are not met - provided that a comment explains why that particular alternative is selected. - PR: https://git.openjdk.java.net/jdk/pull/6568
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Mon, 17 Jan 2022 06:32:13 GMT, Prasadrao Koppula wrote: >> This system property should only be used for TLS, and the CBT can be used in >> both the SPNEGO mechanism and the Kerberos 5 mechanism. Therefore I suggest >> the name should probably contain "tls" (or maybe "https") and "negotiate". >> >> BTW, will you reuse this system property if we decide to support CBT in NTLM >> as well? > > I vote for "jdk.https.tls.cbt" > It's actually a purely system property rather than a Net property at the > moment (same as the other spnego ones). Maybe, I should convert them all to > net properties, so they can be documented/set in that file? AFAICS this file documents properties used by the networking task - not necessarily net properties (e.g. java.net.preferIPv6Addresses is documented there but AFAICT it is a plain system property) - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Sat, 15 Jan 2022 00:49:05 GMT, Weijun Wang wrote:
>> Argh - you're right I missed the fact that the 3 expressions where included
>> in parenthesis. I read it as
>>
>> ! (s.equals("always")) || ...
>
> Shall we log a message if the value is not one of the 3 forms?
Usually malformed values are just ignored - and the property takes its default
value. But yes - s.n.w.h.HttpClient has a logger so it wouldn't be much effort
to log it as a DEBUG trace for better diagnostic.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Thu, 13 Jan 2022 12:10:11 GMT, Michael McMahon wrote:
> Hi,
>
> This change adds Channel Binding Token (CBT) support to HTTPS
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos)
> authentication scheme. When enabled, the implementation preemptively includes
> a CBT with authentication requests over Kerberos. The feature is enabled as
> follows:
>
> A system property "jdk.spnego.cbt" is defined which can have the values
> "never" (default), which means the feature is disabled, "always", which means
> the CBT is included for all https Negotiate authentications, or it can take
> the form "domain:a,b.c,*.d.com" which is a comma separated list of
> domains/hosts where the feature is enabled, and disabled everywhere else. In
> the given example, the CBT would be included in authentication requests for
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
> sub-domains.
>
> A test will be added separately to the implementation.
>
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>
> Thanks,
> Michael
Have you been able to test this on a specific setup?
Would be good to hear from @msheppar too.
src/java.base/share/classes/sun/net/www/http/HttpClient.java line 152:
> 150: * If enabled (for a particular destination) then SPNEGO
> authentication requests will include
> 151: * a channel binding token for the destination server. The default
> behavior and setting for the
> 152: * property is "never"
Maybe this description should be added to
`src/java.base//share/classes/java/net/doc-files/net-properties.html` too?
src/java.security.jgss/share/classes/module-info.java line 36:
> 34: module java.security.jgss {
> 35: requires java.naming;
> 36: requires java.security.sasl;
Someone from security-dev should probably review this and validate that this is
OK. I'm also a bit uncomfortable that we require a class from
`com.sun.jndi.ldap.sasl` even though `java.naming` is already required by
`java.security.jgss` - so maybe this is OK.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8280010: Remove double buffering of InputStream for Properties.load
On Mon, 10 Jan 2022 20:46:36 GMT, Andrey Turbanov wrote: > `Properties.load` uses `java.util.Properties.LineReader`. LineReader already > buffers input stream. Hence wrapping InputStream in BufferedInputStream is > redundant. Changes to `java.util.logging` look fine. - PR: https://git.openjdk.java.net/jdk/pull/7021
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 10:03:37 GMT, Michael McMahon wrote:
>> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 180:
>>
>>> 178: static String normalizeCBT(String s) {
>>> 179: if (s == null || ! (s.equals("always") ||
>>> 180: s.equals("never") || s.startsWith("domain:"))) {
>>
>> I guess there's a `!` missing in front of `s.startsWith("domain:")` here?
>
> This is what was intended (equivalent)
>
> `if (s ==null || (s!="always" && s!="never" && !s.startsWith("domain")))`
Argh - you're right I missed the fact that the 3 expressions where included in
parenthesis. I read it as
! (s.equals("always")) || ...
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Thu, 13 Jan 2022 12:10:11 GMT, Michael McMahon wrote:
> Hi,
>
> This change adds Channel Binding Token (CBT) support to HTTPS
> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos)
> authentication scheme. When enabled, the implementation preemptively includes
> a CBT with authentication requests over Kerberos. The feature is enabled as
> follows:
>
> A system property "jdk.spnego.cbt" is defined which can have the values
> "never" (default), which means the feature is disabled, "always", which means
> the CBT is included for all https Negotiate authentications, or it can take
> the form "domain:a,b.c,*.d.com" which is a comma separated list of
> domains/hosts where the feature is enabled, and disabled everywhere else. In
> the given example, the CBT would be included in authentication requests for
> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its
> sub-domains.
>
> A test will be added separately to the implementation.
>
> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>
> Thanks,
> Michael
src/java.base/share/classes/sun/net/www/http/HttpClient.java line 180:
> 178: static String normalizeCBT(String s) {
> 179: if (s == null || ! (s.equals("always") ||
> 180: s.equals("never") || s.startsWith("domain:"))) {
I guess there's a `!` missing in front of `s.startsWith("domain:")` here?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: JDK-8277795: ldap connection timeout not honoured under contention [v3]
On Thu, 13 Jan 2022 10:09:45 GMT, Daniel Fuchs wrote:
>> src/java.naming/share/classes/com/sun/jndi/ldap/LdapClientFactory.java line
>> 70:
>>
>>> 68: public PooledConnection createPooledConnection(PoolCallback pcb,
>>> long timeout)
>>> 69: throws NamingException {
>>> 70: return new LdapClient(host, port, socketFactory,
>>
>> any need to perform sanity check against erroneous negative values on the
>> timeout supplied here and in other parts of the solution
>
> Hmmm... Good point. I had looked into this yesterday when I reviewed - and
> AFAIU a value <= 0 would be interpreted as no timeout (that is, infinite
> timeout) - and that seems consistent throughout. It's non obvious - but I
> convinced myself that passing a negative value here would not necessarily be
> an error, and would work as expected. However the narrowing down of a
> negative long to an int doesn't necessarily preserve the sign.
> @robm-openjdk the conversion from long to int probably needs to also take
> care of values that are < Integer.MIN_VALUE.
>
>
> jshell> long l = Integer.MIN_VALUE * 2L
> l ==> -4294967296
>
> jshell> int x = (int)l
> x ==> 0
>
> jshell> long l = Integer.MIN_VALUE * 2L + 1
> l ==> -4294967295
>
> jshell> int x = (int)l
> x ==> 1
(Though I don't think it can happen - but maybe I'm mistaken)
In any case it's safer to sanitize the input.
-
PR: https://git.openjdk.java.net/jdk/pull/6568
