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: 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 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 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 [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: 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: 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 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
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: 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: 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: 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: 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: 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: 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: 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: 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: 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: 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: 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: 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: 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: 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: 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: 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
hg: jdk7/tl/jdk: 6651382: The Java JVM SNMP provider reports incorrect stats when asked for multiple OIDs
Changeset: d8b6af0f01f6 Author:dfuchs Date: 2008-03-03 12:29 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/d8b6af0f01f6 6651382: The Java JVM SNMP provider reports incorrect stats when asked for multiple OIDs Summary: The JvmMemPoolEntryImpl must use the row index when caching data. Reviewed-by: jfdenise ! src/share/classes/sun/management/snmp/jvminstr/JvmMemPoolEntryImpl.java
hg: jdk7/tl/jdk: 6673853: LegacyIntrospectorTest is testing an old deprecated com.sun API not present in OpenJDK.
Changeset: b64e68bf6b0b Author:dfuchs Date: 2008-05-29 15:33 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/b64e68bf6b0b 6673853: LegacyIntrospectorTest is testing an old deprecated com.sun API not present in OpenJDK. Summary: Removed test from open test suite - the corresponding deprecated legacy API is not in open source tree Reviewed-by: emcmanus - test/javax/management/Introspector/LegacyIntrospectorTest.java
hg: jdk7/tl/jdk: 6592586: RequiredModelMBean prints a WARNING message when calling getAttributes() for a non-existing attr
Changeset: 6ca4564520e7 Author:dfuchs Date: 2008-05-30 14:35 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/6ca4564520e7 6592586: RequiredModelMBean prints a WARNING message when calling getAttributes() for a non-existing attr Summary: Switched traces to FINER - except when logging fails - in which cases the traces are logged to FINE Reviewed-by: emcmanus ! src/share/classes/javax/management/modelmbean/RequiredModelMBean.java
hg: jdk7/tl/jdk: 6402254: Revisit ModelMBean DescriptorSupport implementation of equals and hashCode.
Changeset: 8c667d55b79e Author:dfuchs Date: 2008-07-29 19:21 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/8c667d55b79e 6402254: Revisit ModelMBean DescriptorSupport implementation of equals and hashCode. Reviewed-by: emcmanus ! src/share/classes/com/sun/jmx/mbeanserver/Util.java ! src/share/classes/javax/management/ImmutableDescriptor.java ! src/share/classes/javax/management/modelmbean/DescriptorSupport.java
hg: jdk7/tl/jdk: 6730926: Document that create/registerMBean can throw RuntimeMBeanException from postRegister
Changeset: 914370f03119 Author:dfuchs Date: 2008-07-31 12:41 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/914370f03119 6730926: Document that create/registerMBean can throw RuntimeMBeanException from postRegister Reviewed-by: emcmanus ! src/share/classes/com/sun/jmx/interceptor/DefaultMBeanServerInterceptor.java ! src/share/classes/javax/management/MBeanRegistration.java ! src/share/classes/javax/management/MBeanServer.java ! src/share/classes/javax/management/MBeanServerConnection.java + test/javax/management/MBeanServer/PostExceptionTest.java
hg: jdk7/tl/jdk: 6689505: Improve MBeanServerNotification.toString
Changeset: 7622f1de1486 Author:dfuchs Date: 2008-07-31 14:20 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/7622f1de1486 6689505: Improve MBeanServerNotification.toString Reviewed-by: emcmanus ! src/share/classes/javax/management/MBeanServerNotification.java + test/javax/management/MBeanServer/MBeanServerNotificationTest.java
hg: jdk7/tl/jdk: 6616825: JMX query returns no value in 1.0 compatibility mode - deserialization bug in readObject()
Changeset: 98caad5c563c Author:dfuchs Date: 2008-07-31 17:38 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/98caad5c563c 6616825: JMX query returns no value in 1.0 compatibility mode - deserialization bug in readObject() Reviewed-by: emcmanus ! src/share/classes/javax/management/ObjectName.java ! test/javax/management/ObjectName/SerialCompatTest.java
hg: jdk7/tl/jdk: 6732192: CORE_PKGS.gmk: need to declare javax.management.event in the CORE_PKGS variable
Changeset: e0dc076d99b8 Author:dfuchs Date: 2008-08-01 11:41 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/e0dc076d99b8 6732192: CORE_PKGS.gmk: need to declare javax.management.event in the CORE_PKGS variable Reviewed-by: emcmanus ! make/docs/CORE_PKGS.gmk
hg: jdk7/tl/jdk: 6733294: MBeans tab - UI issues with writable attributes
Changeset: 233f8854d8b4 Author:dfuchs Date: 2008-08-08 14:24 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/233f8854d8b4 6733294: MBeans tab - UI issues with writable attributes Reviewed-by: emcmanus ! make/netbeans/jconsole/build.properties ! make/netbeans/jconsole/build.xml ! src/share/classes/sun/tools/jconsole/inspector/TableSorter.java ! src/share/classes/sun/tools/jconsole/inspector/XMBeanAttributes.java ! src/share/classes/sun/tools/jconsole/inspector/XPlotter.java ! src/share/classes/sun/tools/jconsole/inspector/XSheet.java ! src/share/classes/sun/tools/jconsole/inspector/XTable.java ! src/share/classes/sun/tools/jconsole/inspector/XTextFieldEditor.java
hg: jdk7/tl/jdk: 5072476: RFE: support cascaded (federated) MBean Servers; ...
Changeset: 9145ff046bb4 Author:dfuchs Date: 2008-09-04 14:46 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/9145ff046bb4 5072476: RFE: support cascaded (federated) MBean Servers 6299231: Add support for named MBean Servers Summary: New javax.management.namespace package. Reviewed-by: emcmanus ! make/docs/CORE_PKGS.gmk ! src/share/classes/com/sun/jmx/defaults/JmxProperties.java ! src/share/classes/com/sun/jmx/interceptor/DefaultMBeanServerInterceptor.java + src/share/classes/com/sun/jmx/interceptor/DispatchInterceptor.java + src/share/classes/com/sun/jmx/interceptor/DomainDispatchInterceptor.java ! src/share/classes/com/sun/jmx/interceptor/MBeanServerInterceptor.java + src/share/classes/com/sun/jmx/interceptor/MBeanServerInterceptorSupport.java - src/share/classes/com/sun/jmx/interceptor/MBeanServerSupport.java + src/share/classes/com/sun/jmx/interceptor/NamespaceDispatchInterceptor.java ! src/share/classes/com/sun/jmx/interceptor/SingleMBeanForwarder.java ! src/share/classes/com/sun/jmx/mbeanserver/JmxMBeanServer.java ! src/share/classes/com/sun/jmx/mbeanserver/MXBeanLookup.java ! src/share/classes/com/sun/jmx/mbeanserver/Repository.java ! src/share/classes/com/sun/jmx/mbeanserver/SunJmxMBeanServer.java ! src/share/classes/com/sun/jmx/mbeanserver/Util.java + src/share/classes/com/sun/jmx/namespace/DomainInterceptor.java + src/share/classes/com/sun/jmx/namespace/HandlerInterceptor.java + src/share/classes/com/sun/jmx/namespace/JMXNamespaceUtils.java + src/share/classes/com/sun/jmx/namespace/NamespaceInterceptor.java + src/share/classes/com/sun/jmx/namespace/ObjectNameRouter.java + src/share/classes/com/sun/jmx/namespace/RoutingConnectionProxy.java + src/share/classes/com/sun/jmx/namespace/RoutingMBeanServerConnection.java + src/share/classes/com/sun/jmx/namespace/RoutingProxy.java + src/share/classes/com/sun/jmx/namespace/RoutingServerProxy.java + src/share/classes/com/sun/jmx/namespace/package.html + src/share/classes/com/sun/jmx/namespace/serial/DefaultRewritingProcessor.java + src/share/classes/com/sun/jmx/namespace/serial/IdentityProcessor.java + src/share/classes/com/sun/jmx/namespace/serial/JMXNamespaceContext.java + src/share/classes/com/sun/jmx/namespace/serial/RewritingProcessor.java + src/share/classes/com/sun/jmx/namespace/serial/RoutingOnlyProcessor.java + src/share/classes/com/sun/jmx/namespace/serial/SerialRewritingProcessor.java + src/share/classes/com/sun/jmx/namespace/serial/package.html ! src/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java ! src/share/classes/com/sun/jmx/remote/util/EventClientConnection.java ! src/share/classes/javax/management/InstanceNotFoundException.java ! src/share/classes/javax/management/MBeanPermission.java ! src/share/classes/javax/management/MBeanServer.java ! src/share/classes/javax/management/MBeanServerDelegate.java ! src/share/classes/javax/management/MBeanServerFactory.java ! src/share/classes/javax/management/ObjectName.java ! src/share/classes/javax/management/event/EventClient.java ! src/share/classes/javax/management/event/EventClientDelegate.java + src/share/classes/javax/management/namespace/JMXDomain.java + src/share/classes/javax/management/namespace/JMXNamespace.java + src/share/classes/javax/management/namespace/JMXNamespaceMBean.java + src/share/classes/javax/management/namespace/JMXNamespacePermission.java + src/share/classes/javax/management/namespace/JMXNamespaceView.java + src/share/classes/javax/management/namespace/JMXNamespaces.java + src/share/classes/javax/management/namespace/JMXRemoteNamespace.java + src/share/classes/javax/management/namespace/JMXRemoteNamespaceMBean.java + src/share/classes/javax/management/namespace/MBeanServerConnectionWrapper.java + src/share/classes/javax/management/namespace/MBeanServerSupport.java + src/share/classes/javax/management/namespace/VirtualEventManager.java + src/share/classes/javax/management/namespace/package-info.java ! src/share/classes/javax/management/remote/JMXConnectorFactory.java ! src/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java + test/javax/management/MBeanServerFactory/NamedMBeanServerTest.java ! test/javax/management/ObjectName/ApplyWildcardTest.java + test/javax/management/namespace/DomainCreationTest.java + test/javax/management/namespace/EventWithNamespaceControlTest.java + test/javax/management/namespace/EventWithNamespaceTest.java + test/javax/management/namespace/ExportNamespaceTest.java + test/javax/management/namespace/JMXDomainTest.java + test/javax/management/namespace/JMXNamespaceSecurityTest.java + test/javax/management/namespace/JMXNamespaceTest.java + test/javax/management/namespace/JMXNamespaceViewTest.java + test/javax/management/namespace/JMXNamespacesTest.java + test/javax/management/namespace/JMXRemoteNamespaceTest.java + test/javax/management/namespace/JMXRemoteTargetNamespace.java + test/javax/management/namespace/LazyDomainTest.java + test/javax/management/namespace/MXBeanRefTest.java + test/javax/manag
hg: jdk7/tl/jdk: 6745832: jmx namespaces: Some refactoring/commenting would improve code readability.
Changeset: 5778303e2e14 Author:dfuchs Date: 2008-09-09 17:01 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/5778303e2e14 6745832: jmx namespaces: Some refactoring/commenting would improve code readability. Reviewed-by: emcmanus ! src/share/classes/com/sun/jmx/interceptor/DefaultMBeanServerInterceptor.java ! src/share/classes/com/sun/jmx/interceptor/DispatchInterceptor.java ! src/share/classes/com/sun/jmx/interceptor/DomainDispatchInterceptor.java ! src/share/classes/com/sun/jmx/interceptor/NamespaceDispatchInterceptor.java ! src/share/classes/com/sun/jmx/mbeanserver/Util.java ! src/share/classes/com/sun/jmx/namespace/DomainInterceptor.java ! src/share/classes/com/sun/jmx/namespace/HandlerInterceptor.java ! src/share/classes/com/sun/jmx/namespace/JMXNamespaceUtils.java ! src/share/classes/com/sun/jmx/namespace/NamespaceInterceptor.java ! src/share/classes/com/sun/jmx/namespace/RoutingConnectionProxy.java ! src/share/classes/com/sun/jmx/namespace/RoutingMBeanServerConnection.java ! src/share/classes/com/sun/jmx/namespace/RoutingProxy.java ! src/share/classes/com/sun/jmx/namespace/RoutingServerProxy.java ! src/share/classes/javax/management/namespace/JMXDomain.java ! src/share/classes/javax/management/namespace/JMXNamespace.java ! src/share/classes/javax/management/namespace/JMXNamespaces.java ! src/share/classes/javax/management/namespace/JMXRemoteNamespace.java ! src/share/classes/javax/management/namespace/MBeanServerConnectionWrapper.java ! test/javax/management/namespace/Wombat.java
hg: jdk7/tl/jdk: 6746754: jmx namespace: test for leading separator missing; ...
Changeset: 3e7b9a0f3a6f Author:dfuchs Date: 2008-09-10 16:27 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/3e7b9a0f3a6f 6746754: jmx namespace: test for leading separator missing 6669137: RFE: InstanceNotFoundException should have a constructor that takes an ObjectName 6746796: jmx namespaces: Several tests are missing an @bug or @run keyword Summary: Note on 6669137: first implementation of 6669137 was actually pushed with 5072476 - here we only have a small update and a test case. Also re-fixes 6737133: Compilation failure of test/javax/management/eventService/LeaseManagerDeadlockTest.java which had failed. Reviewed-by: emcmanus, yjoan ! src/share/classes/com/sun/jmx/namespace/RoutingProxy.java ! src/share/classes/javax/management/InstanceNotFoundException.java + test/javax/management/MBeanServer/InstanceNotFoundExceptionTest.java ! test/javax/management/MBeanServerFactory/NamedMBeanServerTest.java ! test/javax/management/eventService/LeaseManagerDeadlockTest.java ! test/javax/management/namespace/DomainCreationTest.java ! test/javax/management/namespace/EventWithNamespaceControlTest.java ! test/javax/management/namespace/EventWithNamespaceTest.java ! test/javax/management/namespace/ExportNamespaceTest.java ! test/javax/management/namespace/JMXDomainTest.java ! test/javax/management/namespace/JMXNamespaceSecurityTest.java ! test/javax/management/namespace/JMXNamespaceTest.java ! test/javax/management/namespace/JMXNamespaceViewTest.java ! test/javax/management/namespace/JMXNamespacesTest.java ! test/javax/management/namespace/JMXRemoteNamespaceTest.java ! test/javax/management/namespace/LazyDomainTest.java + test/javax/management/namespace/LeadingSeparatorsTest.java ! test/javax/management/namespace/NamespaceCreationTest.java ! test/javax/management/namespace/NamespaceNotificationsTest.java ! test/javax/management/namespace/NullObjectNameTest.java ! test/javax/management/namespace/QueryNamesTest.java ! test/javax/management/namespace/RemoveNotificationListenerTest.java ! test/javax/management/namespace/RoutingServerProxyTest.java ! test/javax/management/namespace/SerialParamProcessorTest.java ! test/javax/management/namespace/SourceNamespaceTest.java ! test/javax/management/namespace/VirtualMBeanNotifTest.java ! test/javax/management/namespace/VirtualMBeanTest.java ! test/javax/management/namespace/VirtualNamespaceQueryTest.java ! test/javax/management/namespace/VirtualPropsTest.java
hg: jdk7/tl/jdk: 6747899: jmx namespaces: hooks for permission checks should be defined in HandlerInterceptor
Changeset: 6a49dd6635ba Author:dfuchs Date: 2008-09-12 17:58 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/6a49dd6635ba 6747899: jmx namespaces: hooks for permission checks should be defined in HandlerInterceptor Reviewed-by: emcmanus ! src/share/classes/com/sun/jmx/namespace/HandlerInterceptor.java ! src/share/classes/com/sun/jmx/namespace/RoutingMBeanServerConnection.java
hg: jdk7/tl/jdk: 6747983: jmx namespace: unspecified self-link detection logic
Changeset: 09a7e38337e9 Author:dfuchs Date: 2008-09-12 19:06 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/09a7e38337e9 6747983: jmx namespace: unspecified self-link detection logic Reviewed-by: emcmanus ! src/share/classes/com/sun/jmx/namespace/NamespaceInterceptor.java ! src/share/classes/javax/management/namespace/JMXRemoteNamespace.java ! test/javax/management/namespace/JMXNamespaceTest.java
hg: jdk7/tl/jdk: 6748745: JConsole: plotters don't resize well when the window is resized
Changeset: 044bfa235270 Author:dfuchs Date: 2008-09-17 13:40 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/044bfa235270 6748745: JConsole: plotters don't resize well when the window is resized Summary: part of the fix was contributed by jfdenise Reviewed-by: jfdenise ! src/share/classes/sun/tools/jconsole/Plotter.java ! src/share/classes/sun/tools/jconsole/inspector/XMBeanAttributes.java ! src/share/classes/sun/tools/jconsole/inspector/XPlotter.java ! src/share/classes/sun/tools/jconsole/inspector/XPlottingViewer.java
hg: jdk7/tl/jdk: 6332953: JMX agent should bind to loopback address when starting the local connector server
Changeset: 6a76dcaf15e3 Author:dfuchs Date: 2008-10-09 14:10 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/6a76dcaf15e3 6332953: JMX agent should bind to loopback address when starting the local connector server Reviewed-by: emcmanus ! src/share/classes/sun/management/jmxremote/ConnectorBootstrap.java + src/share/classes/sun/management/jmxremote/LocalRMIServerSocketFactory.java ! src/share/lib/management/management.properties
hg: jdk7/tl/jdk: 6758165: ConnectorBootstrap.DefaultValues should have a default value for USE_LOCAL_ONLY
Changeset: f50f9b0d18a8 Author:dfuchs Date: 2008-10-10 10:58 +0200 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/f50f9b0d18a8 6758165: ConnectorBootstrap.DefaultValues should have a default value for USE_LOCAL_ONLY Reviewed-by: alanb, emcmanus ! src/share/classes/sun/management/jmxremote/ConnectorBootstrap.java
hg: jdk7/tl/jdk: 6683213: CounterMonitor's derived Gauge badly initialized
Changeset: 67718d2bd49c Author:dfuchs Date: 2008-11-14 17:22 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/67718d2bd49c 6683213: CounterMonitor's derived Gauge badly initialized Reviewed-by: emcmanus ! src/share/classes/javax/management/monitor/CounterMonitor.java ! src/share/classes/javax/management/monitor/GaugeMonitor.java ! src/share/classes/javax/management/monitor/Monitor.java + test/javax/management/monitor/DerivedGaugeMonitorTest.java
hg: jdk7/tl/jdk: 6774170: LocalRMIServerSocketFactory should protect against ServerSocket.accept().getInetAddress() being null
Changeset: 97e2e87aa035 Author:dfuchs Date: 2008-11-21 18:18 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/97e2e87aa035 6774170: LocalRMIServerSocketFactory should protect against ServerSocket.accept().getInetAddress() being null Reviewed-by: emcmanus, jfdenise ! src/share/classes/sun/management/jmxremote/LocalRMIServerSocketFactory.java + test/sun/management/jmxremote/LocalRMIServerSocketFactoryTest.java
hg: jdk7/tl/jdk: 6319823: new mbean register/unregister notification for groups of mbeans; ...
Changeset: a99a2d2f3249 Author:dfuchs Date: 2008-12-04 17:58 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/a99a2d2f3249 6319823: new mbean register/unregister notification for groups of mbeans 6779698: Merge error caused duplicate example code in MBeanServerNotification Reviewed-by: emcmanus ! src/share/classes/javax/management/MBeanServerNotification.java
hg: jdk7/tl/jdk: 6768935: Clarify the behaviour of ObjectName pattern matching with regards to namespaces
Changeset: 61e73bc43e72 Author:dfuchs Date: 2008-12-09 20:20 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/61e73bc43e72 6768935: Clarify the behaviour of ObjectName pattern matching with regards to namespaces Reviewed-by: emcmanus ! src/share/classes/com/sun/jmx/interceptor/DispatchInterceptor.java ! src/share/classes/com/sun/jmx/interceptor/DomainDispatchInterceptor.java ! src/share/classes/com/sun/jmx/interceptor/NamespaceDispatchInterceptor.java ! src/share/classes/com/sun/jmx/mbeanserver/MXBeanLookup.java ! src/share/classes/com/sun/jmx/mbeanserver/Util.java ! src/share/classes/com/sun/jmx/namespace/DomainInterceptor.java ! src/share/classes/com/sun/jmx/namespace/NamespaceInterceptor.java ! src/share/classes/com/sun/jmx/namespace/ObjectNameRouter.java ! src/share/classes/com/sun/jmx/namespace/RoutingMBeanServerConnection.java ! src/share/classes/com/sun/jmx/namespace/RoutingProxy.java ! src/share/classes/javax/management/MBeanServer.java ! src/share/classes/javax/management/MBeanServerConnection.java ! src/share/classes/javax/management/ObjectName.java ! src/share/classes/javax/management/namespace/JMXDomain.java ! src/share/classes/javax/management/namespace/JMXNamespacePermission.java ! src/share/classes/javax/management/namespace/JMXNamespaces.java ! src/share/classes/javax/management/namespace/package-info.java ! test/javax/management/namespace/LeadingSeparatorsTest.java ! test/javax/management/namespace/NullDomainObjectNameTest.java ! test/javax/management/namespace/NullObjectNameTest.java ! test/javax/management/namespace/QueryNamesTest.java
hg: jdk7/tl/jdk: 3 new changesets
Changeset: fa87de6b1ac3 Author:dfuchs Date: 2009-03-12 15:36 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/fa87de6b1ac3 6661448: Make the SNMP agent optional when OPENJDK=true and IMPORT_BINARY_PLUGS=false Reviewed-by: mchung, ohair ! make/com/sun/jmx/Makefile ! make/java/management/Makefile ! make/javax/management/Makefile ! make/sun/management/Makefile ! src/share/classes/sun/management/Agent.java ! test/com/sun/jmx/snmp/SnmpOidHashCode.java ! test/com/sun/jmx/snmp/TimeTicksWrapping.java Changeset: e90ce2ac06a8 Author:dfuchs Date: 2009-03-13 14:25 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/e90ce2ac06a8 Merge - src/share/classes/sun/misc/JavaIODeleteOnExitAccess.java Changeset: ef27484bbd7f Author:dfuchs Date: 2009-03-18 18:55 +0100 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/ef27484bbd7f Merge
RfR - 8080933: LogManager.demandSystemLogger should accept a 'caller' argument.
Hi, Please find below a fix for https://bugs.openjdk.java.net/browse/JDK-8080933 8080933: LogManager.demandSystemLogger should accept a 'caller' argument. http://cr.openjdk.java.net/~dfuchs/webrev_8080933/webrev.00 LogManager.demandLogger accepts a caller argument which is later used to resolve ResourceBundle. However LogManager.demandSystemLogger doesn't have that caller argument - which could later lead to some inconsistency when resource bundles are resolved. As for now this is more an inconsistency than an issue since by definition the caller that creates a system logger is a system class in the bootstrap class loader. However it could become an issue in a modular JDK - where different system classes may come from different modules and where information on the actual system caller class may be needed. best regards, -- daniel
RFR: 8129572 - Cleanup usage of getResourceAsStream in jaxp
Hi, Please find below a patch for 8129572 - Cleanup usage of getResourceAsStream in jaxp https://bugs.openjdk.java.net/browse/JDK-8129572 http://cr.openjdk.java.net/~dfuchs/webrev_8129572/webrev.00/ This removes a bunch of unused getResourceAsStream methods and sanitizes the few that remain. I took the liberty to also add some additional debug traces to the LSSerializerTest.java to help debug when the test fails. best regards, -- daniel
Re: RFR: 8129572 - Cleanup usage of getResourceAsStream in jaxp
On 23/06/15 17:14, Claes Redestad wrote: "priveleged" typo in: http://cr.openjdk.java.net/~dfuchs/webrev_8129572/webrev.00/jaxp/src/java.xml/share/classes/com/sun/org/apache/xpath/internal/functions/FuncSystemProperty.java.udiff.html The line isn't touched by the patch, but might be a nice drive-by fix... Done. Thanks for reviewing! -- daniel
Re: RFR: 8129572 - Cleanup usage of getResourceAsStream in jaxp
On 23/06/15 16:44, Alan Bateman wrote: On 23/06/2015 15:16, Daniel Fuchs wrote: Hi, Please find below a patch for 8129572 - Cleanup usage of getResourceAsStream in jaxp https://bugs.openjdk.java.net/browse/JDK-8129572 http://cr.openjdk.java.net/~dfuchs/webrev_8129572/webrev.00/ This removes a bunch of unused getResourceAsStream methods and sanitizes the few that remain. I took the liberty to also add some additional debug traces to the LSSerializerTest.java to help debug when the test fails. Typo in the comment in FuncSystemProperty.loadPropertyFile: "propery" -> property. Done. Thanks for reviewing! -- daniel Otherwise this looks like good clean-up to me. -Alan
Re: Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules
Hi Frank, The proposed changes look good to me. best regards, -- daniel On 24/06/15 09:58, Frank Yuan wrote: Hi, Would you like to have a review for bug https://bugs.openjdk.java.net/browse/JDK-8080266? This bug is caused by jigsaw change, the context class loader can't load internal resource which is in a named module any more. To fix it, LSSerializerImpl shall invoke ResourceBundle.getBundle(resourceName) instead of ResourceBundle.getBundle(resourceName, locale, classloader) to create CharInfo instance, that will getBundle with the module of the caller(here it's java.xml module). This patch also forces to use the internal XMLEntities.properties because the default xml character entity reference should always be applied. The webrev is at: http://cr.openjdk.java.net/~fyuan/8080266/webrev.00/ Any comment will be appreciated. Thanks, Frank
Re: Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules
Hi Frank, I could push it for you. -- daniel On 6/25/15 5:05 AM, Frank Yuan wrote: So, would you like to push the code for me? Best Regards Frank -Original Message- From: huizhe wang [mailto:huizhe.w...@oracle.com] Sent: Thursday, June 25, 2015 12:57 AM To: Daniel Fuchs Cc: Frank Yuan; 'core-libs-dev'; 'Lance Andersen'; 'jibing chen'; 'Gustavo Galimberti'; sandeep.konch...@oracle.com; 'Alexandre (Shura) Iline'; 'Alan Bateman' Subject: Re: Review request for JDK-8080266: Failed to create CharInfo due to ResourceBundle update for modules +1. -Joe On 6/24/2015 1:58 AM, Daniel Fuchs wrote: Hi Frank, The proposed changes look good to me. best regards, -- daniel On 24/06/15 09:58, Frank Yuan wrote: Hi, Would you like to have a review for bug https://bugs.openjdk.java.net/browse/JDK-8080266? This bug is caused by jigsaw change, the context class loader can't load internal resource which is in a named module any more. To fix it, LSSerializerImpl shall invoke ResourceBundle.getBundle(resourceName) instead of ResourceBundle.getBundle(resourceName, locale, classloader) to create CharInfo instance, that will getBundle with the module of the caller(here it's java.xml module). This patch also forces to use the internal XMLEntities.properties because the default xml character entity reference should always be applied. The webrev is at: http://cr.openjdk.java.net/~fyuan/8080266/webrev.00/ Any comment will be appreciated. Thanks, Frank
RfR: 8129880 - Cleanup usage of Class.getResource in jaxp
Hi, Please find below a cleanup patch for jaxp: https://bugs.openjdk.java.net/browse/JDK-8129880 8129880 - Cleanup usage of getResource in jaxp Class.getResource(classFile) was used for printing the class location for debuging purposes. CodeSource.getLocation() is a better fit for that. http://cr.openjdk.java.net/~dfuchs/webrev_8129880/webrev.00/index.html best regards, -- daniel
RFR - 8129956: jaxp: CodeSource.getLocation() might return null
Hi, It was brought to my attention that CodeSource.getLocation() might return null. The javadoc is not very clear on the subject, leading me to believe it could not. Here is a trivial change (with some trivial additional cleanup) that adds guards against null before calling getLocation().toString(); http://cr.openjdk.java.net/~dfuchs/webrev_8129956/webrev.00/ best regards, -- daniel
Re: RFR - 8129956: jaxp: CodeSource.getLocation() might return null
On 6/27/15 12:57 PM, Peter Levart wrote: Hi Daniel, Just a question. Would diamonds on anonymous instance creation expressions work in these cases? I tried it - it didn't work. You would have to use a local variable for that. cheers, -- daniel Regards, Peter On 06/26/2015 12:45 PM, Daniel Fuchs wrote: Hi, It was brought to my attention that CodeSource.getLocation() might return null. The javadoc is not very clear on the subject, leading me to believe it could not. Here is a trivial change (with some trivial additional cleanup) that adds guards against null before calling getLocation().toString(); http://cr.openjdk.java.net/~dfuchs/webrev_8129956/webrev.00/ best regards, -- daniel
Re: RFR - 8129956: jaxp: CodeSource.getLocation() might return null
On 29/06/15 10:06, Paul Sandoz wrote:
That's odd i would of expected it to work. Here's Joe's patch to changes in the
JDK:
http://cr.openjdk.java.net/~darcy/8078467.0/jdk.patch
(Search for "new PrivilegedAction" in the patch.)
e.g. an expression in java.io.ObjectInputStream:
http://hg.openjdk.java.net/jdk9/dev/jdk/file/93ced310c728/src/java.base/share/classes/java/io/ObjectInputStream.java#l1265
private static boolean auditSubclass(final Class subcl) {
Boolean result = AccessController.doPrivileged(
new PrivilegedAction<>() {
public Boolean run() {
Hmmm... Strange indeed. Maybe I did a mistake - or maybe it had to do
with using return directly without passing through an intermediary
Boolean variable?
NetBeans didn't suggest replacing with diamonds either - which it
usually does - but then maybe it was busy scanning classpath ;-)
best,
-- daniel
Re: RFR - 8129956: jaxp: CodeSource.getLocation() might return null
Thanks Paul.
I have another cleaner patch coming - I'll include those
changes as well.
-- daniel
On 29/06/15 11:13, Paul Sandoz wrote:
On Jun 29, 2015, at 10:35 AM, Daniel Fuchs wrote:
On 29/06/15 10:06, Paul Sandoz wrote:
That's odd i would of expected it to work. Here's Joe's patch to changes in the
JDK:
http://cr.openjdk.java.net/~darcy/8078467.0/jdk.patch
(Search for "new PrivilegedAction" in the patch.)
e.g. an expression in java.io.ObjectInputStream:
http://hg.openjdk.java.net/jdk9/dev/jdk/file/93ced310c728/src/java.base/share/classes/java/io/ObjectInputStream.java#l1265
private static boolean auditSubclass(final Class subcl) {
Boolean result = AccessController.doPrivileged(
new PrivilegedAction<>() {
public Boolean run() {
Hmmm... Strange indeed. Maybe I did a mistake - or maybe it had to do
with using return directly without passing through an intermediary
Boolean variable?
I modified some of those doPriv blocks in:
src/java.xml/share/classes/javax/xml/validation/SecuritySupport.java
and it compiled without issue:
diff -r 17b47acf5b3d
src/java.xml/share/classes/javax/xml/validation/SecuritySupport.java
--- a/src/java.xml/share/classes/javax/xml/validation/SecuritySupport.java
Tue Jun 23 19:50:10 2015 +0200
+++ b/src/java.xml/share/classes/javax/xml/validation/SecuritySupport.java
Mon Jun 29 11:11:26 2015 +0200
@@ -42,9 +42,9 @@
ClassLoader getContextClassLoader() {
-return (ClassLoader)
-AccessController.doPrivileged(new PrivilegedAction() {
-public Object run() {
+return
+AccessController.doPrivileged(new PrivilegedAction<>() {
+public ClassLoader run() {
ClassLoader cl = null;
//try {
cl = Thread.currentThread().getContextClassLoader();
@@ -57,9 +57,9 @@
}
String getSystemProperty(final String propName) {
-return (String)
-AccessController.doPrivileged(new PrivilegedAction() {
-public Object run() {
+return
+AccessController.doPrivileged(new PrivilegedAction<>() {
+public String run() {
return System.getProperty(propName);
}
});
NetBeans didn't suggest replacing with diamonds either - which it
usually does - but then maybe it was busy scanning classpath ;-)
This is a relatively new 9-based language feature, so i don't expect the IDEs
have caught up yet (IntelliJ 15 EAP has not for this feature, or for the
try-with-resources enhancement IIRC).
Paul.
Re: RFR - 8129956: jaxp: CodeSource.getLocation() might return null
On 29/06/15 11:13, Paul Sandoz wrote: NetBeans didn't suggest replacing with diamonds either - which it >usually does - but then maybe it was busy scanning classpath;-) > This is a relatively new 9-based language feature, so i don't expect the IDEs have caught up yet (IntelliJ 15 EAP has not for this feature, or for the try-with-resources enhancement IIRC). Right. That is the issue. The IDE puts a red flag in front of the line saying that it can't infer the type-variable T - but if you do compile then the compilation succeeds. It seems I did put too much trust in the IDE ;-) best regards, -- daniel
RFR: 8130051 Cleanup usage of reflection in jaxp
Hi, Please find below a patch that cleans up some of the reflection usage in jaxp. https://bugs.openjdk.java.net/browse/JDK-8130051 http://cr.openjdk.java.net/~dfuchs/webrev_8130051/webrev.00/ This is not a deep cleanup - it is just a first pass... There may be some opportunity to get rid of some more obsolete code - which can be addressed by followup bugs/RFE (such as JDK-8129892). I also took this opportunity to remove two files which are not used in the JDK. jaxp/test, jdk/test/javax/xml/jaxp, and JCK test suites (api/javax_xml api/xinclude api/xsl xml_schema) passed. best regards, -- daniel
Re: RFR - 8129956: jaxp: CodeSource.getLocation() might return null
On 29/06/15 20:06, huizhe wang wrote: Maybe it's better to live with PrivilegedAction and PrivilegedAction instead? I'm using NetBeans 8.0.2, it looks like it doesn't support "source level" beyond JDK 1.8. It's a bit bothersome to have "red flags". Plus, we won't be able to build with JDK 8. What would you think? OK - let's be NetBeans-8 friendly for now - I also dislike the false positive red flags. We can revisit when IDE support is a bit more mature. cheers, -- daniel
Re: RFR: 8130051 Cleanup usage of reflection in jaxp
Hi Joe, On 30/06/15 00:52, huizhe wang wrote: Hi Daniel, Another great cleanup! The change looks good to me. Only minor suggestion: same as 8129956, would you want to add the types back instead of using diamond operator? It looks good, but the red flags in the NetBeans could be a bit troublesome. Plus, honestly, I still regularly compile jaxp-only using JDK 8 for a quick turnaround. Using JDK 8 makes it easier to compare with results from JDK8 as we work on regressions and etc. Usually diamonds are forever ;-) OK - I will replace the offending diamonds before pushing. Thanks for the review! best regards, -- daniel Thanks, Joe On 6/29/2015 4:17 AM, Daniel Fuchs wrote: Hi, Please find below a patch that cleans up some of the reflection usage in jaxp. https://bugs.openjdk.java.net/browse/JDK-8130051 http://cr.openjdk.java.net/~dfuchs/webrev_8130051/webrev.00/ This is not a deep cleanup - it is just a first pass... There may be some opportunity to get rid of some more obsolete code - which can be addressed by followup bugs/RFE (such as JDK-8129892). I also took this opportunity to remove two files which are not used in the JDK. jaxp/test, jdk/test/javax/xml/jaxp, and JCK test suites (api/javax_xml api/xinclude api/xsl xml_schema) passed. best regards, -- daniel
Re: RFR: 8130051 Cleanup usage of reflection in jaxp
Hi Joe, Paul, On 30/06/15 10:47, Daniel Fuchs wrote: OK - I will replace the offending diamonds before pushing. On 30/06/15 10:49, Paul Sandoz wrote:> I am beseech you not to do that! > > Unless there is a really strong reason why JAXP 8 code should be the > same as JAXP 9 code we should be able to leverage new language and > API features. I don't know of any such reason, so assuming that we > should not treat JAXP code any differently from other JDK code where > such changes have already been made and from which you will get > squiggly red lines. I surrender. Let's keep the shiny diamonds... The patch will go as reviewed then. best regards, -- daniel
RFR- 8130238: Remove com.sun.org.apache.xalan.internal.xsltc.cmdline
Hi, Please find below a trivial patch that removes com.sun.org.apache.xalan.internal.xsltc.cmdline This package is not used in the JDK. http://cr.openjdk.java.net/~dfuchs/webrev_8130238/webrev.00/ Jaxp tests passed locally (jaxp/test, jdk/test/javax/xml/jaxp) JCK for XML APIs also passed. More thorough testing through jprt under way... best regards, -- daniel
RfR - 8130649: java/util/logging/LoggingDeadlock2.java times out
Please find below a changeset to address: 8130649: java/util/logging/LoggingDeadlock2.java times out https://bugs.openjdk.java.net/browse/JDK-8130649 The proposed changeset doesn't fix the issue but instruments the test a bit more so that we can get a clue of what is happening. http://cr.openjdk.java.net/~dfuchs/webrev_8130649/webrev.00/ When a test fails in timeout, jtreg obtains a Thread Dump before killing it. However - in our case - that thread dump is quite uninteresting - because the real test is performed in a subprocess - and that's where bad things could be happening. The proposed instrumentation will try to obtain a thread dump of the subprocess after a sensible time (60secs) but before the jtreg timeout kicks in (default jtreg timeout should be about 120sec x time factor - if I remember well). best regards, -- daniel
Re: Idea to Java ArrayLists
Or Stream.of("1", "2", "3").collect(Collectors.toList()); :-)
-- daniel
On 09/07/15 19:46, Louis Wasserman wrote:
Pavel, what you can do there is new ArrayList<>(Arrays.asList("1", "2",
"3", "4", "5"));
On Thu, Jul 9, 2015 at 10:40 AM Pavel Rappo wrote:
Not quite. You don't have the ability to specify a particular
implementation
(e.g. the thing won't work if what you want is, say, LinkedList)
On 9 Jul 2015, at 18:34, kedar mhaswade
wrote:
Wouldn't Arrays.asList
<
http://docs.oracle.com/javase/8/docs/api/java/util/Arrays.html#asList-T...-
work for you?
Re: [9] RFR 8129833: Need basic tests for rmic
On 13/07/15 14:53, FELIX YANG wrote: Hi Daniel, please help to review the change for 8129833. Issue:https://bugs.openjdk.java.net/browse/JDK-8129833 Patch:http://cr.openjdk.java.net/~fyuan/felix/8129833/ The patch add a new class to try rmic with. It covers a problem of locating the class java.awt.Panel in jigsaw build, see https://bugs.openjdk.java.net/browse/JDK-8129779. Hi Felix, it looks good to me. Would you like me to sponsor it? best regards, -- daniel thanks, -Felix
Re: RFR 8131052 Documentation of AbstractSpliterator refers to forEach rather than forEachRemaining
On 13/07/15 11:19, Paul Sandoz wrote:
Hi
Stefan, thanks.
See below for a patch to the documentation of all abstract spliterators.
Looks good to me Paul.
best regards,
-- daniel
Paul.
diff -r a3175de2e354 src/java.base/share/classes/java/util/Spliterators.java
--- a/src/java.base/share/classes/java/util/Spliterators.java Tue Jun 09
07:10:03 2015 +0100
+++ b/src/java.base/share/classes/java/util/Spliterators.java Mon Jul 13
11:16:24 2015 +0200
@@ -1235,8 +1235,8 @@
* An extending class need only
* implement {@link #tryAdvance(java.util.function.Consumer) tryAdvance}.
* The extending class should override
- * {@link #forEachRemaining(java.util.function.Consumer) forEach} if it can
- * provide a more performant implementation.
+ * {@link #forEachRemaining(java.util.function.Consumer) forEachRemaining}
+ * if it can provide a more performant implementation.
*
* @apiNote
* This class is a useful aid for creating a spliterator when it is not
@@ -1358,8 +1358,8 @@
* To implement a spliterator an extending class need only
* implement {@link #tryAdvance(java.util.function.IntConsumer)}
* tryAdvance}. The extending class should override
- * {@link #forEachRemaining(java.util.function.IntConsumer)} forEach} if it
- * can provide a more performant implementation.
+ * {@link #forEachRemaining(java.util.function.IntConsumer)}
forEachRemaining}
+ * if it can provide a more performant implementation.
*
* @apiNote
* This class is a useful aid for creating a spliterator when it is not
@@ -1468,8 +1468,8 @@
* To implement a spliterator an extending class need only
* implement {@link #tryAdvance(java.util.function.LongConsumer)}
* tryAdvance}. The extending class should override
- * {@link #forEachRemaining(java.util.function.LongConsumer)} forEach} if
it
- * can provide a more performant implementation.
+ * {@link #forEachRemaining(java.util.function.LongConsumer)}
forEachRemaining}
+ * if it can provide a more performant implementation.
*
* @apiNote
* This class is a useful aid for creating a spliterator when it is not
@@ -1578,8 +1578,8 @@
* To implement a spliterator an extending class need only
* implement {@link #tryAdvance(java.util.function.DoubleConsumer)}
* tryAdvance}. The extending class should override
- * {@link #forEachRemaining(java.util.function.DoubleConsumer)} forEach} if
- * it can provide a more performant implementation.
+ * {@link #forEachRemaining(java.util.function.DoubleConsumer)}
forEachRemaining}
+ * if it can provide a more performant implementation.
*
* @apiNote
* This class is a useful aid for creating a spliterator when it is not
On Jul 12, 2015, at 11:56 AM, Stefan Zobel wrote:
Hi all,
I just noticed some typos in the
Spliterators.Abstract(Double/Int/Long)Spliterator Javadoc:
a) The forEachRemaining link label is "forEach" instead of
"forEachRemaining" in all AbstractSpliterators.
b) The primitive AbstractSpliterators Javadoc has a surplus "}" just before
the tryAdvance and forEach link labels.
Stefan
Re: RFR 8131052 Documentation of AbstractSpliterator refers to forEach rather than forEachRemaining
On 16/07/15 12:00, Daniel Fuchs wrote:
On 13/07/15 11:19, Paul Sandoz wrote:
Hi
Stefan, thanks.
See below for a patch to the documentation of all abstract spliterators.
Looks good to me Paul.
Sorry - re reading my mail I see that you have an extreneous '}'
at the three last places - just after the closing ')':
>> + * {@link #forEachRemaining(java.util.function.IntConsumer)}
>> forEachRemaining}
>> + * if it can provide a more performant implementation.
>> + * {@link #forEachRemaining(java.util.function.LongConsumer)}
>> forEachRemaining}
>> + * if it can provide a more performant implementation.
>> + * {@link #forEachRemaining(java.util.function.DoubleConsumer)}
>> forEachRemaining}
>> + * if it can provide a more performant implementation.
-- daniel
best regards,
-- daniel
Paul.
diff -r a3175de2e354
src/java.base/share/classes/java/util/Spliterators.java
--- a/src/java.base/share/classes/java/util/Spliterators.javaTue
Jun 09 07:10:03 2015 +0100
+++ b/src/java.base/share/classes/java/util/Spliterators.javaMon
Jul 13 11:16:24 2015 +0200
@@ -1235,8 +1235,8 @@
* An extending class need only
* implement {@link #tryAdvance(java.util.function.Consumer)
tryAdvance}.
* The extending class should override
- * {@link #forEachRemaining(java.util.function.Consumer) forEach}
if it can
- * provide a more performant implementation.
+ * {@link #forEachRemaining(java.util.function.Consumer)
forEachRemaining}
+ * if it can provide a more performant implementation.
*
* @apiNote
* This class is a useful aid for creating a spliterator when it
is not
@@ -1358,8 +1358,8 @@
* To implement a spliterator an extending class need only
* implement {@link #tryAdvance(java.util.function.IntConsumer)}
* tryAdvance}. The extending class should override
- * {@link #forEachRemaining(java.util.function.IntConsumer)}
forEach} if it
- * can provide a more performant implementation.
+ * {@link #forEachRemaining(java.util.function.IntConsumer)}
forEachRemaining}
+ * if it can provide a more performant implementation.
*
* @apiNote
* This class is a useful aid for creating a spliterator when it
is not
@@ -1468,8 +1468,8 @@
* To implement a spliterator an extending class need only
* implement {@link #tryAdvance(java.util.function.LongConsumer)}
* tryAdvance}. The extending class should override
- * {@link #forEachRemaining(java.util.function.LongConsumer)}
forEach} if it
- * can provide a more performant implementation.
+ * {@link #forEachRemaining(java.util.function.LongConsumer)}
forEachRemaining}
+ * if it can provide a more performant implementation.
*
* @apiNote
* This class is a useful aid for creating a spliterator when it
is not
@@ -1578,8 +1578,8 @@
* To implement a spliterator an extending class need only
* implement {@link #tryAdvance(java.util.function.DoubleConsumer)}
* tryAdvance}. The extending class should override
- * {@link #forEachRemaining(java.util.function.DoubleConsumer)}
forEach} if
- * it can provide a more performant implementation.
+ * {@link #forEachRemaining(java.util.function.DoubleConsumer)}
forEachRemaining}
+ * if it can provide a more performant implementation.
*
* @apiNote
* This class is a useful aid for creating a spliterator when it
is not
On Jul 12, 2015, at 11:56 AM, Stefan Zobel wrote:
Hi all,
I just noticed some typos in the
Spliterators.Abstract(Double/Int/Long)Spliterator Javadoc:
a) The forEachRemaining link label is "forEach" instead of
"forEachRemaining" in all AbstractSpliterators.
b) The primitive AbstractSpliterators Javadoc has a surplus "}" just
before
the tryAdvance and forEach link labels.
Stefan
Re: RFR (jaxp) 8131907 : Numerous threads lock during XML processing
Hi Joe, Looks good to me! nit: the two statics don't agree on style: 'private static final' vs 'private final static' best regards, -- daniel On 20/07/15 23:47, huizhe wang wrote: Please review a change that removes unnecessary class loading. http://cr.openjdk.java.net/~joehw/jdk9/8131907/webrev/ Thanks, Joe
RFR: 8132256: jaxp: Investigate removal of com/sun/org/apache/bcel/internal/util/ClassPath.java
Hi, Please find below a patch that fixes 8132256: jaxp: Investigate removal of com/sun/org/apache/bcel/internal/util/ClassPath.java https://bugs.openjdk.java.net/browse/JDK-8132256 webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8132256/webrev.00 The class com.sun.org.appache.bcel.internal.util.ClassPath does not seem to serve any useful purpose in JDK 9. - There's only one instance created, using its default empty constructor. - There's only one method called on that instance, from SyntheticRepository.java: _path.getInputStream(className); - Because _path was created using the default (and deprecated) empty constructor, and because ClassPath is loaded from the BCL (ClassPath.class.getClassLoader() == null), then _path.getInputStream(className) will always throw an IOException. Therefore - it seems that in the JDK - the use of com.sun.org.appache.bcel.internal.util.ClassPath is reduced to an elaborate means of throwing an IOException. The patch above arranges for the removal of com/sun/org/apache/bcel/internal/util/ClassPath.java best regards, -- daniel
RfR - 8130059: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/EnvironmentCheck.java
Hi, Please find below a fix for yet another cleanup for jaxp: 8130059: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/EnvironmentCheck.java https://bugs.openjdk.java.net/browse/JDK-8130059 http://cr.openjdk.java.net/~dfuchs/webrev_8130059/webrev.00/ EnvironmentCheck doesn't seem to serve any purpose in JDK 9. It is not called anywhere. The proposal is to remove it. By doing a full grep on the JDK I also identified another unused class (Hashtree2Node.java) which referred to EnvironmentCheck inside a comment. I took the liberty to remove that class as well. As for the latter cleanup, what triggered this is that EnvironmentCheck is using sun.boot.class.path... best regards, -- daniel
Re: RfR - 8130059: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/EnvironmentCheck.java
On 28/07/15 19:20, huizhe wang wrote: Hi Daniel, On 7/28/2015 8:22 AM, Daniel Fuchs wrote: Hi, Please find below a fix for yet another cleanup for jaxp: Thanks for yet another cleanup! And, there is a lot more to come :-) 8130059: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/EnvironmentCheck.java https://bugs.openjdk.java.net/browse/JDK-8130059 http://cr.openjdk.java.net/~dfuchs/webrev_8130059/webrev.00/ EnvironmentCheck doesn't seem to serve any purpose in JDK 9. Agree. It'd be a confusion more than anything else if used since it produces many irrelevant information. It is not called anywhere. The proposal is to remove it. By doing a full grep on the JDK I also identified another unused class (Hashtree2Node.java) which referred to EnvironmentCheck inside a comment. I took the liberty to remove that class as well. Ok. The webrev looks good to me. If you'd want to remove the two Version classes as shown in the test, that would be fine with me too. Then you could remove the whole test. Thanks Joe! One of the two version classes (xalan) is used by ...xslt.Process.java I already logged another bug to investigate removing that as well (JDK-8130058). Maybe we should remove the two versions classes as part of that other bug? Or I could update my webrev to just remove the xerces Version.java now, which as far as I can see is not called anywhere. As you prefer :-) cheers, -- daniel Best regards, Joe As for the latter cleanup, what triggered this is that EnvironmentCheck is using sun.boot.class.path... best regards, -- daniel
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with "RuntimeException: Error: poll() returned null; expected ref object"
Hi Kim, I agree with your proposed fix. I see that you have added a comment for future maintainers. Thanks for that - as the implication of the ordering between the two volatile writes is not immediately perceptible to the casual reader. You have good eyes - I hadn't spotted the race condition, but it is definitely there. I have tried to see if the proposed reordering could have other side effects but couldn't find any: The reference might now temporarily appear unenqueued though it's already at the head of the queue - but since 'head' is private in ReferenceQueue and access to the head reference are adequately protected by 'lock' then it looks like this is not going to be an issue. +1 Thanks for finding the cause of the failure and the devising the fix! best regards, -- daniel On 29/07/15 09:57, Kim Barrett wrote: Please review this fix of a race condition in j.l.r.Reference/ReferenceQueue. See comments in the bug report for a description of the race. The race is being fixed by reordering a pair of volatile assignments. CR: https://bugs.openjdk.java.net/browse/JDK-8132306 Webrev: http://cr.openjdk.java.net/~kbarrett/8132306/webrev.00/ Testing: jprt with default and hotspot testsets Locally tested race in original code by insertion of sleep in enqueue and verifying ReferenceEnqueue regression test fails consistently as described in the bug report. Locally tested fixed code by insertion of sleeps at various points and verifying ReferenceEnqueue regression test still passes.
RfR - 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java
Hi, Please find below a patch that removes a bunch of unused files in jdk9/dev/jaxp: https://bugs.openjdk.java.net/browse/JDK-8130058 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.00/ best regards, -- daniel
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with "RuntimeException: Error: poll() returned null; expected ref object"
Hi Peter, I'm glad you're looking at this too! We can't have too many eyes :-) On 30/07/15 10:38, Peter Levart wrote: Suppose we have a Reference 'r' and it's associated ReferenceQueue 'q'. Currently it can happen that the following evaluates to true, which is surprising: q.poll() == null && r.isEnqueued() But on the other hand this can only happen if two different threads are polling the queue - in which case only one of them will get the reference. In such a situation, the symmetric condition would not be surprising (as the other thread would get q.poll() != null): r.isEnqueued() && q.poll() == null The original bug fixed by Kim is more surprising, because there's only one Thread doing the polling, and it does get: r.isEnqueud() && q.poll() == null although nobody else is polling the queue. This should no longer occur in this situation with Kim's fix. cheers, -- daniel
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with "RuntimeException: Error: poll() returned null; expected ref object"
On 30/07/15 12:20, Peter Levart wrote: On 07/30/2015 11:12 AM, Daniel Fuchs wrote: Hi Peter, I'm glad you're looking at this too! We can't have too many eyes :-) On 30/07/15 10:38, Peter Levart wrote: Suppose we have a Reference 'r' and it's associated ReferenceQueue 'q'. Currently it can happen that the following evaluates to true, which is surprising: q.poll() == null && r.isEnqueued() But on the other hand this can only happen if two different threads are polling the queue - in which case only one of them will get the reference. In such a situation, the symmetric condition would not be surprising (as the other thread would get q.poll() != null): r.isEnqueued() && q.poll() == null The original bug fixed by Kim is more surprising, because there's only one Thread doing the polling, and it does get: r.isEnqueud() && q.poll() == null although nobody else is polling the queue. This should no longer occur in this situation with Kim's fix. cheers, -- daniel Yes, these are two different issues. The one Kim has fixed is the race of above expressions with Queue.enqueue() (when the queue is changing from the associated instance to ENQUEUED). The one I'm pointing at and Kim has already identified as potential issue is the race of the following expression: r.isEnqueued() && q.poll() == null && r.isEnqueued() ==> true with Queue.[realy]poll() (when the queue is changing from ENQUEUED to NULL). Which only happens if at least two threads are polling the queue, but is equally surprising and prone to cause bugs, don't you think? Hi Peter, Yes - this is also surprising. Is it prone to cause bugs? possibly - but how serious I'm not sure. Is it 'equally' surprising - well - that was the point of my argument: there are two threads polling the same queue - so one of them should get null... Though I agree that in this case - 'r' should be seen has having changed state... The question for me is whether this should be fixed in the same changeset - or whether we should make it in another changeset... cheers, and thanks for pointing it out! -- daniel Regards, Peter
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with "RuntimeException: Error: poll() returned null; expected ref object"
On 30/07/15 13:37, Peter Levart wrote: poll() returning null by itself is not surprising, but if 'r' appears to be "enqueued" before and after the fact, this is surprising. Agreed - not disputing this. The question for me is whether this should be fixed in the same changeset - or whether we should make it in another changeset... It's a different issue, though very similar. Agreed on both counts :-) -- daniel
Re: RfR - 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java
Hi, Please find below an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.01/ Instead of removing the CLITest.java - I copied Process.java into the test hierarchy, renamed it to ProcessXSLT.java, modified it so that it no longer uses internal APIs, and changed the CLITest.java to use that. The only adherence is with com.sun.org.apache.xml.internal.utils.DefaultErrorHandler that the new ProcessXSLT class tries to instantiate through reflection - as that error handler has access to jaxp internals and can give a better error diagnostic. However - the ProcessXSLT will use its own dummy ErrorHandler if it can't managed to instantiate the internal class. This way - we can keep the test :-) best regards, -- daniel On 29/07/15 17:02, Daniel Fuchs wrote: Hi, Please find below a patch that removes a bunch of unused files in jdk9/dev/jaxp: https://bugs.openjdk.java.net/browse/JDK-8130058 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.00/ best regards, -- daniel
Re: RfR - 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java
On 30/07/15 17:55, huizhe wang wrote: Hi Daniel, On 7/30/2015 6:38 AM, Daniel Fuchs wrote: Hi, Please find below an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.01/ Looks good to me. The only nit is it seems createDefaultErrorHandler method is a dup of createDefaultErrorListener? Yes it is - but the return type is different. Instead of removing the CLITest.java - I copied Process.java into the test hierarchy, renamed it to ProcessXSLT.java, modified it so that it no longer uses internal APIs, and changed the CLITest.java to use that. This is a good idea as it keeps the Process utility around. The only adherence is with com.sun.org.apache.xml.internal.utils.DefaultErrorHandler that the new ProcessXSLT class tries to instantiate through reflection - as that error handler has access to jaxp internals and can give a better error diagnostic. However - the ProcessXSLT will use its own dummy ErrorHandler if it can't managed to instantiate the internal class. This way - we can keep the test :-) Perfect indeed, and if others (Yuri) want to use it, it's readily usable. Thanks Joe! -- daniel best, Joe best regards, -- daniel On 29/07/15 17:02, Daniel Fuchs wrote: Hi, Please find below a patch that removes a bunch of unused files in jdk9/dev/jaxp: https://bugs.openjdk.java.net/browse/JDK-8130058 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.00/ best regards, -- daniel
Re: RfR - 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java
On 30/07/15 18:16, huizhe wang wrote: On 7/30/2015 9:08 AM, Daniel Fuchs wrote: On 30/07/15 17:55, huizhe wang wrote: Hi Daniel, On 7/30/2015 6:38 AM, Daniel Fuchs wrote: Hi, Please find below an updated webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.01/ Looks good to me. The only nit is it seems createDefaultErrorHandler method is a dup of createDefaultErrorListener? Yes it is - but the return type is different. Yes, but didn't see it's used. Good point! Removed. -- daniel Joe Instead of removing the CLITest.java - I copied Process.java into the test hierarchy, renamed it to ProcessXSLT.java, modified it so that it no longer uses internal APIs, and changed the CLITest.java to use that. This is a good idea as it keeps the Process utility around. The only adherence is with com.sun.org.apache.xml.internal.utils.DefaultErrorHandler that the new ProcessXSLT class tries to instantiate through reflection - as that error handler has access to jaxp internals and can give a better error diagnostic. However - the ProcessXSLT will use its own dummy ErrorHandler if it can't managed to instantiate the internal class. This way - we can keep the test :-) Perfect indeed, and if others (Yuri) want to use it, it's readily usable. Thanks Joe! -- daniel best, Joe best regards, -- daniel On 29/07/15 17:02, Daniel Fuchs wrote: Hi, Please find below a patch that removes a bunch of unused files in jdk9/dev/jaxp: https://bugs.openjdk.java.net/browse/JDK-8130058 8130058: jaxp: Investigate removal of com/sun/org/apache/xalan/internal/xslt/Process.java http://cr.openjdk.java.net/~dfuchs/webrev_8130058/webrev.00/ best regards, -- daniel
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with "RuntimeException: Error: poll() returned null; expected ref object"
I vote for 1.
:-)
cheers,
-- daniel
On 7/30/15 9:54 PM, Kim Barrett wrote:
On Jul 30, 2015, at 8:09 AM, David Holmes wrote:
On 30/07/2015 9:57 PM, Peter Levart wrote:
'r' has been enqueued.
Thread-1:
r.isEnqueued() &&
q.poll() == null &&
r.isEnqueued()
Thread-2:
q.poll();
Sequence of actions:
T1: r.isEnqueued() ==> true
T2: q.poll() executed to the following point (see HERE) and 'r' was the
last element in the queue ('head' has been assigned to null):
Yeah thanks - just realized it is that darned unsynchronized "fast-path" again.
What a mess.
It a kind of inverse of the original problem.
Original: don't update reference state to enqueued before the queue is updated
This one: don't update the queue state to empty before the reference state
shows it is de-queued.
So yes the fix here is to move "r.queue = null" to before the assignment to
head.
Bring on the next race ;-)
I agree with everything David said above. Bleh!
So I think I can either:
1. Go ahead with my change + Peter's change.
2. Give this back to core-libs while I step carefully away :-)
I *think* option (1) is at least an improvement. But I completely
missed Peter's race, despite having specifically looked for problems
there, so take my opinion with an appropriate quantity of salt.
Re: RFR: 8132306: java/lang/ref/ReferenceEnqueue.java fails with "RuntimeException: Error: poll() returned null; expected ref object"
On 31/07/15 00:56, Kim Barrett wrote: On Jul 30, 2015, at 5:33 PM, David Holmes wrote: So I think I can either: 1. Go ahead with my change + Peter's change. 2. Give this back to core-libs while I step carefully away :-) I *think* option (1) is at least an improvement. But I completely missed Peter's race, despite having specifically looked for problems there, so take my opinion with an appropriate quantity of salt. I vote for 1 as well. I think we have now given good coverage to the two sources of problems (the two lock-free regions): - reference queue state can be seen while queue state is in transition - queue empty state can be seen while reference state is in transition New webrev, with both changes: http://cr.openjdk.java.net/~kbarrett/8132306/webrev.01/ + 1 Thanks for taking care of this one! PS: you might want to add @bug 8132306 to jdk/test/java/lang/ref/ReferenceEnqueue.java best regards, -- daniel
RfR - 8132550: java/util/logging/LoggingDeadlock2.java times out
Hi, Please find below a fix for: https://bugs.openjdk.java.net/browse/JDK-8132550 8132550: java/util/logging/LoggingDeadlock2.java times out I was able to reproduce the deadlock consistently by inserting some Thread.sleep() statements at the critical places. The webrev still shows that (as commented code) - but I will remove those comments before pushing: http://cr.openjdk.java.net/~dfuchs/webrev_8132550/webrev.00/ This issue here is that LogManager should also use the configurationLock when reading its primordial configuration (and not a simple synchronized(this)). That should remove all uses of synchronized((LogManager)this) from LogManager, and ensure that reset(), readConfiguration(), and ensureLogManagerInitialized() all use the same lock. When we moved to using a ReantrantLock object for configuration I had already considered doing this - but at the time I deemed it unnecessary, thinking you couldn't obtain a reference on LogManager through getLogManager() before it had initialized. But this was counting without System.exit() being called and starting the Cleaner thread before the first call to LogManager.getLogManager() had finished. This introduced a small time window in which the deadlock became possible - at exit time, if something happened to initialize the LogManager right at that time. best regards -- daniel
Re: RFR [9] 8132468: docs: replace tags (obsolete in html5) for java.io, java.lang, java.math
On 03/08/15 11:31, Alexander Stepanov wrote:
Hello,
Could you please review the following fix:
http://cr.openjdk.java.net/~avstepan/8132468/webrev.00/
for
https://bugs.openjdk.java.net/browse/JDK-8132468
Just some cleanup for docs (replacing obsolete "").
Thanks,
Alexander
Hi Alexander,
mostly looks good to me - with afew remarks though.
In some files, such as
src/java.base/share/classes/java/io/Console.java and
src/java.base/share/classes/java/lang/ClassLoader.java
(possibly others) - you're using formatting like:
+* {@link #readLine}.
The is not needed around {@link } - as that should be
the default formatting for {@link } (we use {@linkplain } when we
don't want the code formatting for @link).
src/java.base/share/classes/java/io/File.java
+ *
+ * new File( f.{@link
+ * #toURI() toURI}()).equals( f.{@link
+ * #getAbsoluteFile() getAbsoluteFile}())
+ *
Would that be easier to read as:
* {@code new File(f.}{@link
* #toURI() toURI()}{@code .equals(f.}{@link
* #getAbsoluteFile() getAbsoluteFile()}{@code )}
*
(not sure why the original text has hard spaces - as
we usually don't put any space after an open parenthesis)
Same remark for this a few lines below:
+ *
+ * new {@link #File(java.net.URI) File}( f{@code
+ * .toURI()).equals(} f.{@link #getAbsoluteFile()
getAbsoluteFile}())
+ *
I mean - I don't particularly object but if the goal is to replace
and everywhere - then why not go the full
way down?
The other question is whether would be a better fit than
.
Otherwise looks good!
-- daniel
Re: RFR [9] 8132877: docs: replace tags (obsolete in html5) for java.naming
Hi Alexander,
I had a look at
http://cr.openjdk.java.net/~avstepan/8132877/webrev.01/jdk.patch, and
there's nothing that caught my eye.
However - it would be good if you could generate a specdiff so that
we could verify that no mistake has crept in.
best regards,
-- daniel
On 03/08/15 17:39, Alexander Stepanov wrote:
just in case, the updated webrev:
http://cr.openjdk.java.net/~avstepan/8132877/webrev.01/index.html
On 8/3/2015 4:01 PM, Alexander Stepanov wrote:
P.S. I have also to replace {@link ...} with just {@link
...} in a couple of places here (as in the previous RFR 8132468...)
Regards,
Alexander
On 8/3/2015 3:57 PM, Alexander Stepanov wrote:
Thanks!
On 8/3/2015 3:53 PM, Lance Andersen wrote:
I think this looks ok also.
I agree with Daniel that we have additional clean-up opportunities
throughout we can do to the javadocs, but keeping each change set
more narrow helps reduce the chance of introducing additional errors
Best
Lance
On Aug 3, 2015, at 5:43 AM, Alexander Stepanov
mailto:alexander.v.stepa...@oracle.com>> wrote:
http://cr.openjdk.java.net/~avstepan/8132877/webrev.00/
Lance
Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Re: RFR [9] 8132468: docs: replace tags (obsolete in html5) for java.io, java.lang, java.math
Hi Alexander,
src/java.base/share/classes/java/io/File.java
1.
- *
- * new File( f.{@link #toURI()
toURI}()).equals( f.{@link #getAbsoluteFile()
getAbsoluteFile}())
- *
+ *
+ * {@code new File(f.}{@link
+ * #toURI() toURI}{@code ()).equals(f.}{@link
+ * #getAbsoluteFile() getAbsoluteFile}{@code ())}
+ *
Sorry I missed the in my previous suggestion - so
wrapping the whole thing with is probably the better idea,
something like:
*
* new File( f.{@link
* #toURI() toURI()}.equals( f.{@link
* #getAbsoluteFile() getAbsoluteFile()}
*
Whatever you chose to do - please do verify how the new javadoc
looks when changing such complex formatted statements.
2.
- *
- * new {@link #File(java.net.URI)
File}( f.toURI()).equals( f.{@link
#getAbsoluteFile() getAbsoluteFile}())
- *
+ *
+ * {@code new }{@link #File(java.net.URI) File}{@code
.toURI()).equals(f.}
+ * {@link #getAbsoluteFile() getAbsoluteFile}{@code ())}
+ *
As Stuart noted this second change is wrong - it's missing
(f.
I also believe we should keep the original italics
around 'f'.
Can you please generate a specdiff so that we can verify that
there's nothing wrong with the new javadoc?
It is very easy to miss something with such changes...
best regards,
-- daniel
On 03/08/15 11:31, Alexander Stepanov wrote:
Hello,
Could you please review the following fix:
http://cr.openjdk.java.net/~avstepan/8132468/webrev.00/
for
https://bugs.openjdk.java.net/browse/JDK-8132468
Just some cleanup for docs (replacing obsolete "").
Thanks,
Alexander
Re: RFR [9] 8132877: docs: replace tags (obsolete in html5) for java.naming
Hi Alexander,
On 04/08/15 19:34, Alexander Stepanov wrote:
Hello Daniel,
The review was re-uploaded as specdiff indeed discovered a couple of
unwanted changes (in 'InitialContext' and 'ReferralException'), so your
and Pavel's recommendations were very useful, thanks.
webrev (please update the web-page):
http://cr.openjdk.java.net/~avstepan/8132877/webrev.01/index.html
sdiff out (0 changes at the moment):
http://cr.openjdk.java.net/~avstepan/8132877/specdiff.01/overview-summary.html
Looks good for me then.
best regards,
-- daniel
Regards,
Alexander
On 8/4/2015 11:56 AM, Daniel Fuchs wrote:
Hi Alexander,
I had a look at
http://cr.openjdk.java.net/~avstepan/8132877/webrev.01/jdk.patch, and
there's nothing that caught my eye.
However - it would be good if you could generate a specdiff so that
we could verify that no mistake has crept in.
best regards,
-- daniel
On 03/08/15 17:39, Alexander Stepanov wrote:
just in case, the updated webrev:
http://cr.openjdk.java.net/~avstepan/8132877/webrev.01/index.html
On 8/3/2015 4:01 PM, Alexander Stepanov wrote:
P.S. I have also to replace {@link ...} with just {@link
...} in a couple of places here (as in the previous RFR 8132468...)
Regards,
Alexander
On 8/3/2015 3:57 PM, Alexander Stepanov wrote:
Thanks!
On 8/3/2015 3:53 PM, Lance Andersen wrote:
I think this looks ok also.
I agree with Daniel that we have additional clean-up opportunities
throughout we can do to the javadocs, but keeping each change set
more narrow helps reduce the chance of introducing additional
errors
Best
Lance
On Aug 3, 2015, at 5:43 AM, Alexander Stepanov
mailto:alexander.v.stepa...@oracle.com>> wrote:
http://cr.openjdk.java.net/~avstepan/8132877/webrev.00/
<http://cr.openjdk.java.net/%7Eavstepan/8132877/webrev.00/>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance
Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>
Re: RFR [9] 8132468: docs: replace tags (obsolete in html5) for java.io, java.lang, java.math
Hi Alexander, On 8/5/15 2:01 PM, Alexander Stepanov wrote: Hello Stuart, Please see the final version of File.java (please update the page) - just replacing with in the mentioned places: http://cr.openjdk.java.net/~avstepan/8132468/webrev.01/src/java.base/share/classes/java/io/File.java.udiff.html The specdiff reports for lang and io: http://cr.openjdk.java.net/~avstepan/8132468/specdiff.io/overview-summary.html http://cr.openjdk.java.net/~avstepan/8132468/specdiff.lang/overview-summary.html This looks good to me as well. best regards, -- daniel Regards, Alexander On 8/4/2015 12:47 AM, Stuart Marks wrote: Hi Alexander, I think Pavel's advice to run specdiff is useful. It's not too difficult to run and in this case it provides a useful cross-check for making a large number of markup changes. Contact me directly if you need help with specdiff. I've run specdiff over your webrev.01 and I've posted the results here: http://cr.openjdk.java.net/~smarks/reviews/8132468/specdiff.01/overview-summary.html They show that of all the changes you made, only four are considered significant from specdiff's point of view. Of these, two appear to be merely whitespace issues, but there are two changes that do appear significant: File(URI) // constructor File.toURI() In both cases the italic 'f' is intended to be a metasyntactic variable, not an actual variable that's part of the code. The italic 'f' should be restored. In the case of File.toURI() it appears that the resulting code is now malformed, so there's clearly an error here. You and Daniel Fuchs had an exchange about this a bit earlier. I suspect the reason for the that appears between the opening parenthesis and the italic 'f' is so that the tail of the 'f' doesn't crash into parenthesis. In other words, the is there merely for typographical purposes. I suspect that after you restore the italic 'f' it'll look bad, so you'll want to restore the as well. s'marks On 8/3/15 9:03 AM, Alexander Stepanov wrote: Hello Pavel > Hi Alexander, if I were you I would run specdiff Thanks; but sorry, I'm afraid I haven't enough time for the extra experiments just now... > It's very easy to go the all the way and lose oneself in there :) please accept my condolences :) Regards, Alexander On 8/3/2015 6:33 PM, Pavel Rappo wrote: On 3 Aug 2015, at 16:07, Alexander Stepanov wrote: Please see the updated webrev: http://cr.openjdk.java.net/~avstepan/8132468/webrev.01/ removed wrapping around the links (mostly PrintStream.java, PrintWriter.java, File.java), plus other changes in File.java Thanks, Alexander Hi Alexander, if I were you I would run specdiff against the changes and upload it along with the RFR. P.S. "I've tried it once, it was awful." [1] It's very easy to go the all the way and lose oneself in there :) -- [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-December/030262.html -Pavel
Re: RFR [9] 8133040: docs: replace tags (obsolete in html5) for java.management
Hi Alexander, Cursory inspection of the patch file has not revealed anything wrong. Specdiff looks OK, so that's good for me. best regards, -- daniel On 05/08/15 19:37, Alexander Stepanov wrote: > javax.management - one (expected) change in TabularDataSupport: (full package diff: http://cr.openjdk.java.net/~avstepan/8133040/javax.management/overview-summary.html) On 8/5/2015 8:22 PM, Alexander Stepanov wrote: Hello, Could you please review the following fix: http://cr.openjdk.java.net/~avstepan/8133040/webrev.00 for https://bugs.openjdk.java.net/browse/JDK-8133040 specdiffs: java.lang.management - no changes detected: http://cr.openjdk.java.net/~avstepan/8133040/java.lang.management/overview-summary.html javax.management - one (expected) change in TabularDataSupport: http://cr.openjdk.java.net/~avstepan/8133040/javax.management/openmbean/package-summary.html Regards, Alexander
Re: RFR 9: 8133022: Instant.toEpochMilli() silently overflows
Hi Roger, Not a math expert, but this looks good to me :-) best regards, -- daniel On 06/08/15 17:33, Roger Riggs wrote: Please review a small fix and test for Instant.toEpochMilli ArithmeticOverflow. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-overflow-8133022/ Issue: https://bugs.openjdk.java.net/browse/JDK-8133022 Thanks, Roger
Re: RFR <8064470 > JNI exception pending in jdk/src/java/base/unix/native/libjava/FileDescriptor_md.c
Hi Vyom, Thanks for taking care of this issue. This looks good to me as well. I can sponsor this change for you. best regards, -- daniel On 01/09/15 10:38, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8064470 : JNI exception pending in jdk/src/java/base/unix/native/libjava/FileDescriptor_md.c Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8064470/webrev.01/ This change ensure that we immediately return if an exception has been raised (result == NULL) by GetMethodId() or proceed to the next line if there is no JNI pending exception. if result is NULL, there is a pending exception, and we return. if result is not NULL, there is no pending exception, and we proceed to the next line... Thanks, Vyom
Re: RFR 8080486: JNI exception pending in jdk/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c
Hi Vyom, I'm not a net/JNI expert but what you are proposing looks good to me too. Ivan has already given his assent. Unless I hear objections - or comments from other reviewers, I will sponsor this change and push it for you (I'll wait for Monday). best regards, -- daniel On 02/09/15 15:55, Ivan Gerasimov wrote: This looks good. In general, it might be more appropriate to review this on net-...@openjdk.java.net alias. Sincerely yours, Ivan On 02.09.2015 16:22, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080486 : JNI exception pending in jdk/src/java.base/windows/native/libnet/DualStackPlainSocketImpl.c Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080486/webrev.01 This change ensure that we immediately return if an exception has been raised (result == NULL) by GetMethodId/NewGlobalRef or proceed to the next statement if there is no JNI pending exception. Thanks, Vyom
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Hi Vyom, I will sponsor that and push it for you. best regards, -- daniel On 07/09/15 18:52, Alan Bateman wrote: On 07/09/2015 14:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ I think this looks okay now. -Alan
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
Oh - sorry - I hadn't seen Mark question. I will wait until those are resolved... On 07/09/15 19:24, Daniel Fuchs wrote: Hi Vyom, I will sponsor that and push it for you. best regards, -- daniel On 07/09/15 18:52, Alan Bateman wrote: On 07/09/2015 14:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ I think this looks okay now. -Alan
Re: RFR 8080402: File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java
On 09/09/15 17:54, Mark Sheppard wrote: Hi Vyom, yes, I believe the consensus is to proceed with the changes. OK - I will be pushing them then. best regards, -- daniel regards Mark On 09/09/2015 16:23, Vyom Tewari wrote: Hi Mark, Is it OK to go ahead with the patch as it is, or you are expecting any additional modification is required ?. Thanks, Vyom On 9/8/2015 6:35 PM, Mark Sheppard wrote: that's true, the documentation is a bit nebulous on this issue. but the inference is that the file descriptors are indeterminate state. some older man pages allude to this as per solaris man pages, close will " If close() is interrupted by a signal that is to be caught, it will return -1 with errno set to EINTR and the state of fildes is unspecified" if dup2(s, fd) is viewed as a combination of close(fd) and fcntl (s, F_DUP2FD, fd), and close is not restartable then similar semantics could be inferred for dup2 in a EINTR scenario? presume that subsequent call in the RESTARTABLE loop will return another error. On 08/09/2015 09:28, Chris Hegarty wrote: On 7 Sep 2015, at 17:41, Mark Sheppard wrote: a couple of other considerations in the context of this issue perhaps? in this s is being duped onto fd, and part of the dup2 operation is the closing of fd, but what's is the expected state of file descriptor fd in the event of a dup2 failure? I’m not sure that the documentation for dup2 gives us enough detail here. The only situation I can see where fd would not be closed is when EBADF is returned. Should not be an issue here. s is closed in any case, but what about fd, should it be attended to if dup2 < 0 and closed ? is it still considered a valid fd? what can be said about the state of the encapsulating FileDescriptor associated with fd? at this stage can it still be considered valid? should valid() return false? Probably, but may not be worth bothering with unless there are operations that can access it after this method throws. -Chris. regards Mark On 07/09/2015 14:29, Ivan Gerasimov wrote: Thanks! It looks good to me now. Sincerely yours, Ivan On 07.09.2015 16:08, Vyom Tewari wrote: Hi All, Please find the latest diff, which contains the latest fix. http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.02/ Thanks, Vyom On 9/7/2015 3:48 PM, Alan Bateman wrote: On 07/09/2015 10:26, Vyom Tewari wrote: Hi everyone, Can you please review my changes for below bug. Bug: JDK-8080402 : File Leak in jdk/src/java.base/share/classes/sun/net/sdp/SdpSupport.java Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8080402/webrev.01/ This change ensure that if close() fails we throw correct io exception and there is no file leak. close isn't restartable so I think we need to look at that while we are here. -Alan.
RFR: 8033661: readConfiguration does not cleanly reinitialize the logging system
Hi, Please find below a candidate fix for: 8033661: readConfiguration does not cleanly reinitialize the logging system https://bugs.openjdk.java.net/browse/JDK-8033661 http://cr.openjdk.java.net/~dfuchs/webrev_8033661/webrev.00/ LogManager.readConfiguration() presents a number of flaws that may not be fixable without endangering backward compatibility, or stability. Some of them have been quite aptly summed up in JDK-8033661. One of the main issue with readConfiguration() is that it calls reset(). This means that there's no way we can look at the previous configuration and next configuration and close only those handlers that the new configuration does not keep, leaving the others unchanged. I have tried experimenting with adding a per-logger 'handlersInitialized' flag, so that handlers could be created on-demand when a logger first accesses them - as the root logger does, but rapidly abandoned this idea due to the complexity of avoiding race conditions and potential deadlocks. There are also more complex cases that the current readConfiguration() doen't take into account: when a new logger is created by user/library code, the configuration is looked up and any intermediate logger for which there is a configuration (level, handler, useParentHandler) is created: this is done by processParentHandler()). However, readConfiguration() does not ensure that this also happens for existing loggers: if a configuration (e.g a level, or a handler) appears in the new configuration, for the parent of an existing logger, but that parent does not exist yet - then this piece of configuration will be ignored until the parent is created explicitly by application/library code, which might be never. This leaves the logger tree in an inconsistent state with regard to the configuration loaded. It seems that to fix that - we should call processParentHandler() on all existing loggers. I have the feeling that trying to fix all of this in readConfiguration() could be dangerous WRT to the assumptions that already existing code that calls readConfiguration() may have on what readConfiguration() actually does. If we wanted to fix readConfiguration() we therefore might need to introduce yet another opt-in/opt-out flag for the new behavior. All this leads to propose a more prudent approach. I'm proposing to leave readConfiguration() unchanged, and instead introduce a new updateConfiguration() method that would make it possible to give more control to the caller on how the old and new configuration should be merged. The new updateConfiguration() method would take a remapper function, allowing the caller to control how the old and new configuration are going to be merged into a resulting configuration. The method would ensure that loggers are properly initialized, given the difference between their old & resulting configuration, calling processParentHandlers when necessary. The new method has the advantage that it would not call reset(), as readConfiguration() did. So if a logger has an identical configuration in the old & new conf, it doesn't need to be changed at all. Its handlers don't need to be closed and reopened - they can just be left alone. The problems exhibited by JDK-8033661 could then be solved by calling updateConfiguration() with a remapper function that, for a given property 'p' with old value 'o' and new value 'n' always return the new value 'n': manager.updateConfiguration((p) -> ((o,n) -> n)); comments welcomed, -- daniel
Re: RFR: 8033661: readConfiguration does not cleanly reinitialize the logging system
Hi Mandy,
Thanks a lot for the feedback!
On 13/09/15 00:44, Mandy Chung wrote:
On Sep 9, 2015, at 9:55 AM, Daniel Fuchs wrote:
Hi,
Please find below a candidate fix for:
8033661: readConfiguration does not cleanly reinitialize the
logging system
https://bugs.openjdk.java.net/browse/JDK-8033661
http://cr.openjdk.java.net/~dfuchs/webrev_8033661/webrev.00/
LogManager.readConfiguration() presents a number of flaws that may not
be fixable without endangering backward compatibility, or stability.
Some of them have been quite aptly summed up in JDK-8033661.
Thanks for the detailed analysis. I agree that adding a new method to
reinitialize the logging configuration is the right thing to do.
Have you considered keeping the same method name, readConfiguration with
the new remapper parameter? The downside is the difference that the
reset is not invoked.
No - I didn't consider it. The algorithm implemented by the new
method is quite different from what was implemented in the
previous readConfiguration() methods - so the idea of overloading
readConfiguration() didn't come to my mind.
It might not matter because as you add in @apiNote that the existing
readConfiguration method may be overridden for custom LogManager but
discouraged to be used by applications.
Related question: should the existing readConfiguration methods be
deprecated and recommend to use the new method(s)?
What I see is that the old readConfiguration does the appropriate job
when called for the first time, before any logger has been created.
There is probably some application out there that override it to
install their own configuration.
For this reason I am not sure if it should be deprecated.
Can custom LogManager call updateConfiguration in their constructor
instead of overriding readConfiguration?
I believe that would be bad - it would go against what we've been
trying to do with https://bugs.openjdk.java.net/browse/JDK-8023168
If the existing readConfiguration methods should be deprecated,
i think keeping the same method name may be a better choice.
I'll leave the decision for that (keeping the same name) to you.
I'm not sure we can deprecate the old methods.
IMHO it is difficult to deprecate a method that is actually
called internally by LogManager, and stop calling it would be
risky WRT to compatibility.
1749 * Updates an existing configuration.
We should specify what “existing configuration” is.
If “java.util.logging.config.class” is set and it’s instantiated successfully,
readConfiguration will not read from any configuration file.
updateConfiguration only reads the config file regardless if
“java.util.logging.config.class” is set.
Right. That's a good point.
1761 * @param remapper
“re” probably not needed here. I think “mapper” should work.
OK
Use @throws instead of @exception
OK
VisitedLogger class:
1714 public static VisitedLoggers of(Map visited) {
1715 assert visited == null || visited instanceof IdentityHashMap;
1716 return visited == null ? NEVER : new VisitedLoggers(visited);
1717 }
Instead of doing the assert, what about changing the parameter type to
IdentityHashMap?
can do.
Is VisitedLoggers class necessary? Would it be more explicit to readers to use
a Set (it doesn’t need to be a map) that each logger is visited once?
We don't have an IdentityHashSet - which is the reason while I'm using
an IdentityHashMap here.
ConfigurationProperties class: This enum class is also a Predicate and
it has several static methods for updateConfiguratoin to use e.g.
final Stream allKeys = updatePropertyNames.stream()
.filter(ConfigurationProperties::isOf)
.filter(k -> ConfigurationProperties.isChanging(k, previous, next));
I have to read the comment and follow the implementation of these methods to
figure
out what it’s going on here. It could be simplified and made the code easily
to tell what are being filtered here.
It seems to me that you want to define LevelProperty, HandlerProperty,
UseParentProperty types,
each of which will handle any change (as it’s currently done in the switch
statement).
yes - which is why the enum is useful - as it allows to model all
these.
ConfigurationProperties.ALL - why not use ConfigurationProperties.values()?
Well - I could reverse the question :-)
best regards,
-- daniel
Mandy
Re: RFR: 8033661: readConfiguration does not cleanly reinitialize the logging system
Hi Mandy,
On 13/09/15 00:44, Mandy Chung wrote:
Have you considered keeping the same method name, readConfiguration
with the new remapper parameter? The downside is the difference that
the reset is not invoked. It might not matter because as you add in
@apiNote that the existing readConfiguration method may be overridden
for custom LogManager but discouraged to be used by applications.
I have reworked the API notes...
The class level documentation says:
*
* If the "java.util.logging.config.class" property is set, then the
* property value is treated as a class name. The given class will be
* loaded, an object will be instantiated, and that object's constructor
* is responsible for reading in the initial configuration. (That object
* may use other system properties to control its configuration.) The
* alternate configuration class can use {@code
readConfiguration(InputStream)}
* to define properties in the LogManager.
So with that in mind, I have slightly altered the @apiNotes:
in readConfiguration():
* @apiNote {@code readConfiguration} is principally useful for
*establishing the LogManager primordial configuration.
*
*Calling this method directly from the application code after the
*LogManager has been initialized is discouraged.
etc...
The rationale is that an application which needs to establish
a custom configuration should probably use the
{@code "java.util.logging.config.class"} property, and
call LogManager.readConfiguration(InputStream) from there,
as hinted in the class-level documentation.
in readConfiguration(InputStream):
* @apiNote {@code readConfiguration} is principally useful for
applications
*which use the {@code "java.util.logging.config.class"} property
*to establish their own custom configuration.
*
*Calling this method directly from the application code after the
*LogManager has been initialized is discouraged.
Same rationale than above. After thinking it over I'm not
sure overriding readConfiguration is something we should
encourage. I would prefer some wording that does not imply
it :-)
1749 * Updates an existing configuration.
We should specify what “existing configuration” is.
If “java.util.logging.config.class” is set and it’s instantiated successfully,
readConfiguration will not read from any configuration file.
updateConfiguration only reads the config file regardless
if “java.util.logging.config.class” is set.
I updated the doc for updateConfiguration(mapper):
* Updates an existing configuration.
*
* @implSpec
* This is equivalent to calling:
*
* try (final InputStream in = new
FileInputStream()) {
* final BufferedInputStream bin = new BufferedInputStream(in);
* updateConfiguration(bin, mapper);
* }
*
* where {@code } is the logging configuration
file path.
*
* Note that this method not take into account the value of the
* {@code "java.util.logging.config.class"} property.
* The {@code "java.util.logging.config.file"} system property is read
* to find the logging properties file, whether the
* {@code "java.util.logging.config.class"} property is set or not.
* If the {@code "java.util.logging.config.file"} system property
is not
* defined then the default configuration is used.
* The default configuration is typically loaded from the
properties file
* "{@code conf/logging.properties}" in the Java installation
directory.
*
in updateConfiguration(InputStream, mapper), after the table:
*
* Note that this method has no special handling for the "config"
property.
* The new value provided by the mapper function will be stored in the
* LogManager properties, but {@code updateConfiguration} will not
parse its
* value nor instantiate the classes it points to.
1761 * @param remapper
“re” probably not needed here. I think “mapper” should work.
OK. Should I still use the term of 'remapping function' then?
Or does that become 'mapping function' too?
Here is the new webrev - with the other comments from your previous
mail also integrated:
http://cr.openjdk.java.net/~dfuchs/webrev_8033661/webrev.01/
best regards,
-- daniel
Re: RFR 8073542 : File Leak in jdk/src/java/base/unix/native/libnet/PlainDatagramSocketImpl.c
Hi Vyom, I will sponsor your change and push the fix for you. I'll sync up with Rob to check that we don't step on each other's toes. best regards, -- daniel On 16/09/15 16:03, Chris Hegarty wrote: The changes look good to me Vyom. -Chris. On 16/09/15 10:08, Vyom Tewari wrote: Hi All, Please review my changes for below bug. Bug: JDK-8073542 : File Leak in jdk/src/java/base/unix/native/libnet/PlainDatagramSocketImpl.c Webrev: http://cr.openjdk.java.net/~dfuchs/vyom/8073542/webrev.00 This change ensure that if "setsocketopt" fails we close the corresponding fd and throw correct exception. Thanks, Vyom
