Re: RFR: 8287008: Improve tests for thread dumps in JSON format [v2]

[Daniel Fuchs]

On Fri, 20 May 2022 11:32:14 GMT, Alan Bateman  wrote:

>> This is a test-only change to add some test infrastructure and improve the 
>> testing of thread dumps in JSON format. The new tests added by JEP 425 for 
>> this thread dump format search the JSON text for strings but don't parse it 
>> completely.  These tests can be improved with a test class that parses the 
>> thread dump. The tests for JEP 428 like make use of the test infrastructure 
>> too.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add example thread dump and usage to class description

test/lib/jdk/test/lib/threaddump/ThreadDump.java line 82:

> 80:  * "parent": "",
> 81:  * "owner": null,
> 82:  * "threads:": [...],

probably the extra colon after `threads` is a typo here

-

PR: https://git.openjdk.java.net/jdk/pull/8784

Re: RFR: 8284209: Replace remaining usages of 'a the' in source code

[Daniel Fuchs]

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: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]

[Daniel Fuchs]

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: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable [v2]

[Daniel Fuchs]

On Thu, 31 Mar 2022 08:03:23 GMT, Andrey Turbanov  wrote:

>> Method `Class.isAssignableFrom` is often used in form of:
>> 
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>> 
>> if (clazz.isInstance(obj)) {
>> 
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>> 
>> I tried to measure performance via JMH
>> 
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>> 
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>> 
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>> 
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>> 
>> `isInstance` is a bit faster than `isAssignableFrom`
>> 
>> Benchmark  Mode  Cnt  Score   Error  Units
>> integerDouble_isAssignableFrom avgt5  1,173 ± 0,026  ns/op
>> integerDouble_isInstance   avgt5  0,939 ± 0,038  ns/op
>> integerIntegerDouble_isAssignableFrom  avgt5  2,106 ± 0,068  ns/op
>> numberIntegerDouble_isInstance avgt5  1,516 ± 0,046  ns/op
>> integerInteger_isAssignableFromavgt5  1,175 ± 0,029  ns/op
>> integerInteger_isInstance  avgt5  0,886 ± 0,017  ns/op
>> numberDouble_isAssignableFrom  avgt5  1,172 ± 0,007  ns/op
>> numberDouble_isInstanceavgt5  0,891 ± 0,030  ns/op
>> numberInteger_isAssignableFrom avgt5  1,169 ± 0,014  ns/op
>> numberInteger_isInstance   avgt5  0,887 ± 0,016  ns/op
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8280035: Use Class.isInstance instead of Class.isAssignableFrom where 
> applicable
>   apply suggestion to avoid second Method.invoke call

src/java.desktop/share/classes/javax/imageio/spi/ServiceRegistry.java line 230:

> 228: List l = new ArrayList<>();
> 229: for (Class c : categoryMap.keySet()) {
> 230: if (c.isInstance(provider)) {

Can this be reached if `provider` is null? If yes there could be a change of 
behaviour as the previous code would have thrown NPE.

-

PR: https://git.openjdk.java.net/jdk/pull/7061

Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v3]

[Daniel Fuchs]

On Thu, 28 Apr 2022 01:34:19 GMT, Joe Darcy  wrote:

>> To enable more complete doclint checking (courtesy @jonathan-gibbons), 
>> please review this PR to add type-level @param tags where they are missing.
>> 
>> To the maintainers of java.util.concurrent, those changes could be separated 
>> out in another bug if that would ease maintenance of that code.
>> 
>> Making these library fixes is a blocker for correcting and expanding the 
>> doclint checks (JDK-8285496).
>> 
>> I'll update copyright years before pushing.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to more review feedback.

Thanks for the updates Joe. Your new wording looks good to me.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8410

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Daniel Fuchs]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

Thanks for the refresh Alan! Things look good to me now. 
I have gone over java.io, the networking parts of java.nio, java.net, and the 
JMX related changes.
For what I have reviewed, I believe this is good to go.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v7]

[Daniel Fuchs]

On Tue, 26 Apr 2022 17:27:35 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69

test/jdk/java/net/vthread/HttpALot.java line 76:

> 74: var address = server.getAddress();
> 75: URL url = new URL("http://; + address.getHostName() + ":" + 
> address.getPort() + "/hello");
> 76: 

Nit: Ideally we should use the URIBuilder from the test library here, and the 
IP literal address to improve stability of the test on machines that may have 
strange /etc/hosts configuration. This can be taken care of after this PR has 
been integrated.

test/jdk/java/net/vthread/InterruptHttp.java line 88:

> 86: InetAddress lb = InetAddress.getLoopbackAddress();
> 87: listener = new ServerSocket(0, -1, lb);
> 88: address = "http://; + lb.getHostAddress() + ":" + 
> listener.getLocalPort();

Same remark about using URIBuilder here. It would take care of properly 
formatting the address in case of IPv6 literals. This can be taken care of 
after this PR has been integrated.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v7]

[Daniel Fuchs]

On Tue, 26 Apr 2022 17:27:35 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69

src/jdk.management/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java
 line 149:

> 147:  * access to the file or {@link 
> java.lang.management.ManagementPermission
> 148:  * ManagementPermission("control")} is denied
> 149:  * @since 19

Maybe there ought to be an `@throws UnsupportedOperationException` here since 
that is what the default implementation of the method is supposed to do.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces

[Daniel Fuchs]

On Tue, 26 Apr 2022 22:24:26 GMT, Joe Darcy  wrote:

> To enable more complete doclint checking (courtesy @jonathan-gibbons), please 
> review this PR to add type-level @param tags where they are missing.
> 
> To the maintainers of java.util.concurrent, those changes could be separated 
> out in another bug if that would ease maintenance of that code.
> 
> Making these library fixes is a blocker for correcting and expanding the 
> doclint checks (JDK-8285496).
> 
> I'll update copyright years before pushing.

Hi Joe, just two suggestions about the javax.management changes. Otherwise 
looks good!

src/java.management/share/classes/javax/management/openmbean/ArrayType.java 
line 96:

> 94:  *
> 95:  * @param  the Java type that instances described by this type must
> 96:  * have.

Instead of "instances" - would it be more correct to say "array elements"?

src/java.management/share/classes/javax/management/openmbean/SimpleType.java 
line 60:

> 58:  * @param  the Java type that instances described by this type must
> 59:  * have.
> 60:  *

I would suggest to say "that values described by this type"...

-

PR: https://git.openjdk.java.net/jdk/pull/8410

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v7]

[Daniel Fuchs]

On Tue, 26 Apr 2022 17:27:35 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69

src/java.base/share/classes/sun/nio/ch/DatagramChannelImpl.java line 770:

> 768: synchronized (p) {
> 769: DatagramPackets.setLength(p, n);
> 770: p.setSocketAddress(sender);

Hmmm... For integrity it might be better to call the public  
`DatagramPacket::setData(byte[] buf, int offset, int length)` method here now. 
The additional advantage is that the private access to  
`DatagramPackets.setLength(p, n);` will no longer be needed.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8285366: Fix typos in serviceability

[Daniel Fuchs]

On Thu, 21 Apr 2022 14:03:39 GMT, Daniel Fuchs  wrote:

>> I ran `codespell` on modules owned by the serviceability team 
>> (`java.instrument java.management.rmi java.management jdk.attach 
>> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi 
>> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and 
>> accepted those changes where it indeed discovered real typos.
>> 
>> 
>> I will update copyright years using a script before pushing (otherwise like 
>> every second change would be a copyright update, making reviewing much 
>> harder).
>> 
>> The long term goal here is to make tooling support for running `codespell`. 
>> The trouble with automating this is of course all false positives. But 
>> before even trying to solve that issue, all true positives must be fixed. 
>> Hence this PR.
>
> LGTM. I spotted one fix in a exception message. I don't expect that there 
> will be tests depending on that, but it might still be a good idea to run the 
> serviceability tests to double check. Although I guess the test would have 
> had the same typo and would have been fixed too were it the case :-)

> @dfuch I have only updated files in `src`, so if the incorrect spelling is 
> tested, that test will now fail. I'm unfortunately not well versed in what 
> tests cover serviceability code. Can you suggest a suitable set of tests to 
> run?

Folks from serviceability-dev will be more able to answer that - but I would 
suggest running tier2-tier3 as a precaution.

-

PR: https://git.openjdk.java.net/jdk/pull/8334

Re: RFR: 8285366: Fix typos in serviceability

[Daniel Fuchs]

On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie  wrote:

> I ran `codespell` on modules owned by the serviceability team 
> (`java.instrument java.management.rmi java.management jdk.attach 
> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi 
> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted 
> those changes where it indeed discovered real typos.
> 
> 
> I will update copyright years using a script before pushing (otherwise like 
> every second change would be a copyright update, making reviewing much 
> harder).
> 
> The long term goal here is to make tooling support for running `codespell`. 
> The trouble with automating this is of course all false positives. But before 
> even trying to solve that issue, all true positives must be fixed. Hence this 
> PR.

LGTM. I spotted one fix in a exception message. I don't expect that there will 
be tests depending on that, but it might still be a good idea to run the 
serviceability tests to double check. Although I guess the test would have had 
the same typo and would have been fixed too were it the case :-)

-

PR: https://git.openjdk.java.net/jdk/pull/8334

Re: RFR: 8284161: Implementation of Virtual Threads (Preview)

[Daniel Fuchs]

On Tue, 12 Apr 2022 13:02:44 GMT, Alan Bateman  wrote:

>> src/java.base/share/classes/java/io/BufferedReader.java line 101:
>> 
>>> 99:  */
>>> 100: public BufferedReader(Reader in, int sz) {
>>> 101: Objects.requireNonNull(in);
>> 
>> Not sure if that even matters - but there will be a slight change of 
>> behaviour here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`. 
>> Instead of synchronizing on `in`, the `BufferedReader` will synchronize on 
>> `this`.
>> Now that I think of it - it probably does matter since even if 
>> CAN_USE_INTERNAL_LOCK is true,  untrusted subclasses of BufferedReader 
>> calling this constructor might expect the locking to be performed on `in`?
>
>> Not sure if that even matters - but there will be a slight change of 
>> behaviour here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`. 
>> Instead of synchronizing on `in`, the `BufferedReader` will synchronize on 
>> `this`.
> 
> Good!  We can change this so that depends on whether BufferedReader is 
> extended and whether the given Reader is trusted. It's not clear if anyone 
> could reliably depend on undocumented behavior like this but we have to be 
> cautious at the same time.

Thanks - the same issue appears with `BufferedWriter`/`Writer`.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview)

[Daniel Fuchs]

On Fri, 8 Apr 2022 13:43:39 GMT, Alan Bateman  wrote:

> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
> JDK version to target.
> 
> We will refresh this PR periodically to pick up changes and fixes from the 
> loom repo.
> 
> Most of the new mechanisms in the HotSpot VM are disabled by default and 
> require running with `--enable-preview` to enable.
> 
> The patch has support for x64 and aarch64 on the usual operating systems 
> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for zero 
> and some of the other ports. Additional ports can be contributed via PRs 
> against the fibers branch in the loom repo.
> 
> There are changes in many areas. To reduce notifications/mails, the labels 
> have been trimmed down for now to hotspot, serviceability and core-libs. 
> We'll add the complete set of labels when the PR is further along.
> 
> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
> Doug Lea's CVS. These changes will probably be proposed and integrated in 
> advance of this PR.
> 
> The changes include some non-exposed and low-level infrastructure to support 
> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
> make life a bit easier and avoid having to separate VM changes and juggle 
> branches at this time.

src/java.base/share/classes/java/io/BufferedReader.java line 101:

> 99:  */
> 100: public BufferedReader(Reader in, int sz) {
> 101: Objects.requireNonNull(in);

Not sure if that even matters - but there will be a slight change of behaviour 
here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`. Instead of 
synchronizing on `in`, the `BufferedReader` will synchronize on `this`.
Now that I think of it - it probably does matter since even if 
CAN_USE_INTERNAL_LOCK is true,  untrusted subclasses of BufferedReader calling 
this constructor might expect the locking to be performed on `in`?

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8283254: Remove redundant class jdk/internal/agent/spi/AgentProvider

[Daniel Fuchs]

On Tue, 22 Mar 2022 21:19:20 GMT, Kevin Walls  wrote:

> There are no uses of jdk/internal/agent/spi/AgentProvider, since the SNMP 
> agent was removed ( 8071367 ): this class should be removed.  It is not a 
> public interface.
> 
> Remove 
> src/jdk.management.agent/share/classes/jdk/internal/agent/spi/AgentProvider.java
> Remove import from 
> src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java
> Remove uses from src/jdk.management.agent/share/classes/module-info.java
> 
> Testing with the test/jdk/sun/management tests looks good.

Thanks for taking care of that Kevin!

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7912

Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation

[Daniel Fuchs]

On Tue, 15 Mar 2022 20:22:16 GMT, Kevin Walls  wrote:

> Removing permission checks which, in the presence of a Security Manager, 
> would check for a RuntimePermission "className.subclass".  This was to 
> prevent subclassing these classes, but is no longer necessary with strong 
> encapsulation from modules.

Marked as reviewed by dfuchs (Reviewer).

src/java.management/share/classes/sun/management/spi/PlatformMBeanProvider.java 
line 233:

> 231: }
> 232: return null;
> 233: }

I have verified in module-info.java that sun.management.spi is only 
conditionally exported so I agree that the explicit permission check can now be 
removed.

src/jdk.management.agent/share/classes/jdk/internal/agent/spi/AgentProvider.java
 line 35:

> 33: 
> 34: /**
> 35:  * Instantiates a new AgentProvider.

This class should probably be removed altogether. I see that you logged 
[JDK-8283254](https://bugs.openjdk.java.net/browse/JDK-8283254) so this is fine.

-

PR: https://git.openjdk.java.net/jdk/pull/7827

Re: RFR: 8280400: JDK 19 L10n resource files update - msgdrop 10

[Daniel Fuchs]

On Thu, 10 Mar 2022 17:00:09 GMT, Naoto Sato  wrote:

> IIRC, localized resource files should have the same copyright year as the 
> base English one. That's what I was told by the l10n engineer when I had the 
> same comment.

Thanks Naoto! I have no objection then.

-

PR: https://git.openjdk.java.net/jdk/pull/7765

Re: RFR: 8280400: JDK 19 L10n resource files update - msgdrop 10

[Daniel Fuchs]

On Wed, 9 Mar 2022 21:09:30 GMT, Alisen Chung  wrote:

> msg drop for jdk19, Mar 9, 2022

For simple webserver resource files - should the copyright year be 2022?

-

PR: https://git.openjdk.java.net/jdk/pull/7765

Re: RFR: 6779701: Wrong defect ID in the code of test LocalRMIServerSocketFactoryTest.java

[Daniel Fuchs]

On Mon, 7 Feb 2022 17:28:29 GMT, Kevin Walls  wrote:

> Trivial comment and exception text update, correcting a bug ID to make more 
> sense.

LGTM

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7371

Re: RFR: 8076089: Cleanup: Inline & remove sun.management.Util.newException

[Daniel Fuchs]

On Wed, 26 Jan 2022 04:35:58 GMT, Jaikiran Pai  wrote:

> Can I please get a review for this cleanup that's requested in 
> https://bugs.openjdk.java.net/browse/JDK-8076089?
> 
> The change here removes a package private method 
> `sun.management.Util.newException(Exception e)` and inlines its 
> implementation at the caller locations.
> 
> Given the nature of this change no new jtreg tests have been added.

LGTM

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7226

Re: RFR: JDK-8280492: Address remaining doclint issues in JDK build

[Daniel Fuchs]

On Sat, 22 Jan 2022 21:09:03 GMT, Joe Darcy  wrote:

> Use presumed syntax that will be introduced by JDK-8280488.

Marked as reviewed by dfuchs (Reviewer).

LGTM. I hope in the future IDEs will pick that rule up and offer some help when 
writing `{@link }` `@see`...

-

PR: https://git.openjdk.java.net/jdk/pull/7189

Re: RFR: 8280010: Remove double buffering of InputStream for Properties.load

[Daniel Fuchs]

On Mon, 10 Jan 2022 20:46:36 GMT, Andrey Turbanov  wrote:

> `Properties.load` uses `java.util.Properties.LineReader`. LineReader already 
> buffers input stream. Hence wrapping InputStream in BufferedInputStream is 
> redundant.

Changes to `java.util.logging` look fine.

-

PR: https://git.openjdk.java.net/jdk/pull/7021

Re: RFR: 8275322: Change nested classes in java.management to static nested classes

[Daniel Fuchs]

On Fri, 15 Oct 2021 06:43:13 GMT, Andrey Turbanov  wrote:

> Non-static classes hold a link to their parent classes, which can be avoided.

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5962

Re: RFR: 8274318: Replace 'for' cycles with iterator with enhanced-for in java.management [v2]

[Daniel Fuchs]

On Fri, 1 Oct 2021 21:12:57 GMT, Andrey Turbanov 
 wrote:

>> There are a few places in code, where manual `for` loop is used with 
>> Iterator to iterate over Collection.
>> Instead of manual `for` cycles, it's preferred to use enhanced-for cycle 
>> instead: it's less verbose, makes code easier to read and it's less 
>> error-prone.
>> It doesn't have any performance impact: javac compiler generates similar 
>> code when compiling enhanced-for cycle.
>> Similar cleanups:
>> 1. [JDK-8274016](https://bugs.openjdk.java.net/browse/JDK-8274016) 
>> java.desktop
>> 2. [JDK-8274237](https://bugs.openjdk.java.net/browse/JDK-8274237) java.base
>> 3. [JDK-8274261](https://bugs.openjdk.java.net/browse/JDK-8274261) jdk.jcmd
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8274318: Replace 'for' cycles with iterator with enhanced-for in 
> java.management
>   small code-style fixes

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5695

Re: RFR: 8274757: Cleanup unnecessary calls to Throwable.initCause() in java.management module

[Daniel Fuchs]

On Thu, 16 Sep 2021 20:45:36 GMT, Andrey Turbanov 
 wrote:

> Pass cause exception as constructor parameter is shorter and easier to read.

Nice simplification.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5552

Re: RFR: 8274464: Remove redundant stream() call before forEach in java.* modules

[Daniel Fuchs]

On Wed, 15 Sep 2021 07:12:25 GMT, Andrey Turbanov 
 wrote:

> 8274464: Remove redundant stream() call before forEach in java.* modules

LGTM

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5520

Re: RFR: 8274168: Avoid String.compareTo == 0 to check String equality in java.management [v2]

[Daniel Fuchs]

On Thu, 23 Sep 2021 08:00:25 GMT, Andrey Turbanov 
 wrote:

>> Cleanup places, where String.compareTo is used to check String's equality.
>> Instead String.equals or switch expression could be used. They are faster 
>> and code is cleaner.
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8274168: Avoid String.compareTo == 0 to check String equality in 
> java.management
>   call compareTo only once

Looks reasonable to me too.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5640

Re: RFR: 8272805: Avoid looking up standard charsets [v2]

[Daniel Fuchs]

On Sun, 22 Aug 2021 23:02:06 GMT, Sergey Bylokhov  wrote:

>> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
>> 
>> In many places standard charsets are looked up via their names, for example:
>> absolutePath.getBytes("UTF-8");
>> 
>> This could be done more efficiently(up to x20 time faster) with use of 
>> java.nio.charset.StandardCharsets:
>> absolutePath.getBytes(StandardCharsets.UTF_8);
>> 
>> The later variant also makes the code cleaner, as it is known not to throw 
>> UnsupportedEncodingException in contrary to the former variant.
>> 
>> This change includes:
>>  * demo/utils
>>  * jdk.xx packages
>>  * Some places were missed in the previous changes. I have found it by 
>> tracing the calls to the Charset.forName() by executing tier1,2,3 and 
>> desktop tests.
>> 
>> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
>> 
>> Code excluded in this fix: the Xerces library(should be fixed upstream), 
>> J2DBench(should be compatible to 1.4), some code in the network(the change 
>> there are not straightforward, will do it later).
>> 
>> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.
>
> Sergey Bylokhov 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 14 additional 
> commits since the last revision:
> 
>  - Update the usage of Files.readAllLines()
>  - Update datatransfer
>  - Merge branch 'master' into standard-encodings-in-non-public-modules
>  - Merge branch 'master' into standard-encodings-in-non-public-modules
>  - Fix related imports
>  - Merge branch 'master' into standard-encodings-in-non-public-modules
>  - Cleanup UnsupportedEncodingException
>  - Update PacketStream.java
>  - Rollback TextTests, should be compatible with jdk1.4
>  - Rollback TextRenderTests, should be compatible with jdk1.4
>  - ... and 4 more: 
> https://git.openjdk.java.net/jdk/compare/db47f6e1...e7127644

Changes to http server look good to me.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5210

Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules

[Daniel Fuchs]

On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov  wrote:

> This is the continuation of JDK-8233884 and JDK-8271456. This change affects 
> fewer cases so I fix all "java." modules at once.
> 
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
> 
> This could be done more efficiently(up to x20 time faster) with use of 
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
> 
> The later variant also makes the code cleaner, as it is known not to throw 
> UnsupportedEncodingException in contrary to the former variant.
> 
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5063

Re: RFR: 8272120: Avoid looking for standard encodings in "java." modules

[Daniel Fuchs]

On Tue, 10 Aug 2021 05:08:54 GMT, Sergey Bylokhov  wrote:

> This is the continuation of JDK-8233884 and JDK-8271456. This change affects 
> fewer cases so I fix all "java." modules at once.
> 
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
> 
> This could be done more efficiently(up to x20 time faster) with use of 
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
> 
> The later variant also makes the code cleaner, as it is known not to throw 
> UnsupportedEncodingException in contrary to the former variant.
> 
> tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.

>From my pure developer's perspective the proposed changes look good. If the 
>performance concerns are removed I'd say it looks good to me.

-

PR: https://git.openjdk.java.net/jdk/pull/5063

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Daniel Fuchs]

On Sat, 26 Jun 2021 23:55:46 GMT, Weijun Wang  wrote:

>> src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java 
>> line 53:
>> 
>>> 51: private static final long CURRENT_PID = 
>>> AccessController.doPrivileged(
>>> 52: (PrivilegedAction) 
>>> ProcessHandle::current).pid();
>>> 53: 
>> 
>> The original code separated out the declaration of the PrivilegedAction to 
>> avoid this cast. If you move the code from the original static initializer 
>> into a static method that it called from initializer then it might provide 
>> you with a cleaner way to refactor this. There are several other places in 
>> this patch that could do with similar cleanup.
>
> This cast is only to tell the compiler which overloaded method to call, and I 
> don't think there will be a real cast at runtime. It might look a little ugly 
> but extracting it into a variable declaration/definition plus a new 
> `initStatic` method seems not worth doing, IMHO.

Why not simply declare a local variable in the static initializer below?


private static final long CURRENT_PID;
private static final boolean ALLOW_ATTACH_SELF;
static {
PrivilegedAction pa = ProcessHandle::current;
@SuppressWarnings("removal")
long pid = AccessController.doPrivileged(pa).pid();
CURRENT_PID = pid;
String s = VM.getSavedProperty("jdk.attach.allowAttachSelf");
ALLOW_ATTACH_SELF = "".equals(s) || Boolean.parseBoolean(s);
}

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v5]

[Daniel Fuchs]

On Fri, 25 Jun 2021 11:48:52 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my changes for the removal of the legacy 
>> `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations?
>> 
>> In JDK 13, JEP 353 provided a drop in replacement for the legacy 
>> `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` 
>> implementation was no longer used but included a mitigation mechanism to 
>> reduce compatibility risks in the form of a JDK-specific property 
>> `jdk.net.usePlainSocketImpl` allowing to switch back to the old 
>> implementation. 
>> Similarly, in JDK 15, JEP 373 provided a new implementation for 
>> `DatagramSocket` and `MulticastSocket`, with a JDK-specific property 
>> `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back 
>> to the old implementation in case of compatibility issue.
>> 
>> As these implementations (and the mechanisms they use to enable them to 
>> mitigate compatibility issues) have been deemed no longer necessary, they 
>> now represent a maintenance burden. This patch looks at removing them from 
>> the JDK.
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8253119: Fixed typo

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4574

Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]

[Daniel Fuchs]

On Fri, 25 Jun 2021 10:56:15 GMT, Alan Bateman  wrote:

>> Patrick Concannon has updated the pull request with a new target base due to 
>> a merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains five additional 
>> commits since the last revision:
>> 
>>  - 8253119: Added message to null check in java/net/DatagramSocket
>>  - Merge remote-tracking branch 'origin/master' into JDK-8253119
>>  - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from 
>> java/net/DatagramSocket
>>  - Merge remote-tracking branch 'origin/master' into JDK-8253119
>>  - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl 
>> implementation
>
> src/java.base/share/classes/java/net/DatagramSocket.java line 1398:
> 
>> 1396: DatagramSocketImpl impl = 
>> factory.createDatagramSocketImpl();
>> 1397: Objects.requireNonNull(impl,
>> 1398: "Implementation returned by installed 
>> DatagramSocketFactoryImpl is null");
> 
> I think you mean "DatagramSocketImplFactory" instead of 
> "DatagramSocketFactoryImpl" here.

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

-

PR: https://git.openjdk.java.net/jdk/pull/4574

Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v4]

[Daniel Fuchs]

On Fri, 25 Jun 2021 10:25:52 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my changes for the removal of the legacy 
>> `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations?
>> 
>> In JDK 13, JEP 353 provided a drop in replacement for the legacy 
>> `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` 
>> implementation was no longer used but included a mitigation mechanism to 
>> reduce compatibility risks in the form of a JDK-specific property 
>> `jdk.net.usePlainSocketImpl` allowing to switch back to the old 
>> implementation. 
>> Similarly, in JDK 15, JEP 373 provided a new implementation for 
>> `DatagramSocket` and `MulticastSocket`, with a JDK-specific property 
>> `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back 
>> to the old implementation in case of compatibility issue.
>> 
>> As these implementations (and the mechanisms they use to enable them to 
>> mitigate compatibility issues) have been deemed no longer necessary, they 
>> now represent a maintenance burden. This patch looks at removing them from 
>> the JDK.
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains five additional 
> commits since the last revision:
> 
>  - 8253119: Added message to null check in java/net/DatagramSocket
>  - Merge remote-tracking branch 'origin/master' into JDK-8253119
>  - 8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from 
> java/net/DatagramSocket
>  - Merge remote-tracking branch 'origin/master' into JDK-8253119
>  - 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl 
> implementation

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4574

Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v3]

[Daniel Fuchs]

On Thu, 24 Jun 2021 11:44:19 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my changes for the removal of the legacy 
>> `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations?
>> 
>> In JDK 13, JEP 353 provided a drop in replacement for the legacy 
>> `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` 
>> implementation was no longer used but included a mitigation mechanism to 
>> reduce compatibility risks in the form of a JDK-specific property 
>> `jdk.net.usePlainSocketImpl` allowing to switch back to the old 
>> implementation. 
>> Similarly, in JDK 15, JEP 373 provided a new implementation for 
>> `DatagramSocket` and `MulticastSocket`, with a JDK-specific property 
>> `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back 
>> to the old implementation in case of compatibility issue.
>> 
>> As these implementations (and the mechanisms they use to enable them to 
>> mitigate compatibility issues) have been deemed no longer necessary, they 
>> now represent a maintenance burden. This patch looks at removing them from 
>> the JDK.
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8253119: Removed redundant comments and USE_PLAINDATAGRAMSOCKET from 
> java/net/DatagramSocket

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4574

Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation [v3]

[Daniel Fuchs]

On Thu, 24 Jun 2021 14:22:21 GMT, Alan Bateman  wrote:

>> I've created an issue to track this: 
>> https://bugs.openjdk.java.net/browse/JDK-8269288
>
>> I've created an issue to track this: 
>> https://bugs.openjdk.java.net/browse/JDK-8269288
> 
> Thanks. So are you keeping the Objects.requireNonNull here? If so then it 
> should probably be the 2-arg version so that the message is clear that the 
> custom DatagramSocketFactory returned null, otherwise it will look like a DS 
> bug.

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

-

PR: https://git.openjdk.java.net/jdk/pull/4574

Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation

[Daniel Fuchs]

On Wed, 23 Jun 2021 14:32:22 GMT, Alan Bateman  wrote:

>> Yes - the constructor would throw NPE - but there's no need to call this 
>> constructor if `impl` is null here - it's better to fail early.
>
> This is probably a case where the spec needs say that an error (maybe an 
> unspecified error) is thrown if the custom factory's createDatagramSocketImpl 
> returns null. We can create a separate issue for that as it's probably 
> existed forever.

@AlanBateman 

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

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

-

PR: https://git.openjdk.java.net/jdk/pull/4574

Re: RFR: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation

[Daniel Fuchs]

On Wed, 23 Jun 2021 13:05:24 GMT, Alan Bateman  wrote:

>> Hi,
>> 
>> Could someone please review my changes for the removal of the legacy 
>> `PlainSocketImpl` and `PlainDatagramSocketImpl` implementations?
>> 
>> In JDK 13, JEP 353 provided a drop in replacement for the legacy 
>> `PlainSocketImpl` implementation. Since JDK 13, the `PlainSocketImpl` 
>> implementation was no longer used but included a mitigation mechanism to 
>> reduce compatibility risks in the form of a JDK-specific property 
>> `jdk.net.usePlainSocketImpl` allowing to switch back to the old 
>> implementation. 
>> Similarly, in JDK 15, JEP 373 provided a new implementation for 
>> `DatagramSocket` and `MulticastSocket`, with a JDK-specific property 
>> `jdk.net.usePlainDatagramSocketImpl` also allowing the user to switch back 
>> to the old implementation in case of compatibility issue.
>> 
>> As these implementations (and the mechanisms they use to enable them to 
>> mitigate compatibility issues) have been deemed no longer necessary, they 
>> now represent a maintenance burden. This patch looks at removing them from 
>> the JDK.
>> 
>> Kind regards,
>> Patrick
>
> src/java.base/share/classes/java/net/DatagramSocket.java line 1405:
> 
>> 1403: // create legacy DatagramSocket delegate
>> 1404: DatagramSocketImpl impl = 
>> factory.createDatagramSocketImpl();
>> 1405: Objects.requireNonNull(impl);
> 
> I assume this Objects.requireNonNull is not needed or maybe you want to 
> detect a buggy factory rather than in the the constructor?

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

-

PR: https://git.openjdk.java.net/jdk/pull/4574

Re: RFR: Merge jdk17 [v2]

[Daniel Fuchs]

On Mon, 14 Jun 2021 15:58:15 GMT, Jesper Wilhelmsson  
wrote:

>> Forwardport JDK 17 -> JDK 18
>
> Jesper Wilhelmsson 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.

Looked at this two changesets and they were fine.
 - 8268342: java/foreign/channels/TestAsyncSocketChannels.java fails with 
"IllegalStateException: This segment is already
 - 8268555: Update HttpClient tests that use ITestContext to jtreg 6+1

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4484

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Daniel Fuchs]

On Fri, 21 May 2021 20:37:30 GMT, Weijun Wang  wrote:

>> The code change refactors classes that have a `SuppressWarnings("removal")` 
>> annotation that covers more than 50KB of code. The big annotation is often 
>> quite faraway from the actual deprecated API usage it is suppressing, and 
>> with the annotation covering too big a portion it's easy to call other 
>> deprecated methods without being noticed.
>> 
>> The code change shows some common solutions to avoid such too wide 
>> annotations:
>> 
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local 
>> variable and add annotation on the declaration, and then assign that value 
>> to the original l-value.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update FtpClient.java

Thanks for taking in my suggestions for FtpClient. I have also reviewed the 
changes to java.util.logging and JMX. I wish there was a way to handle 
doPrivileged returning void more nicely. Maybe component maintainers will be 
able to deal with them on a case-by-case basis later on.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4138

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]

[Daniel Fuchs]

On Fri, 21 May 2021 14:00:39 GMT, Weijun Wang  wrote:

>> The code change refactors classes that have a `SuppressWarnings("removal")` 
>> annotation that covers more than 50KB of code. The big annotation is often 
>> quite faraway from the actual deprecated API usage it is suppressing, and 
>> with the annotation covering too big a portion it's easy to call other 
>> deprecated methods without being noticed.
>> 
>> The code change shows some common solutions to avoid such too wide 
>> annotations:
>> 
>> 1. Extract calls into a method and add annotation on that method
>> 2. Assign the return value of a deprecated method call to a new local 
>> variable and add annotation on the declaration, and then assign that value 
>> to the original l-value.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   typo on windows

src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:

> 118: vals[1] = 
> Integer.getInteger("sun.net.client.defaultConnectTimeout", 
> 300_000).intValue();
> 119: return System.getProperty("file.encoding", 
> "ISO8859_1");
> 120: }

It is a bit strange that "file.encoding" seem to get a special treatment - but 
I guess that's OK.

src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:

> 548:  * @throws IOException if the connection was unsuccessful.
> 549:  */
> 550: @SuppressWarnings("removal")

Could the scope of the annotation be further reduced by applying it to the two 
places where `doPrivileged` is called in this method?

src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 921:

> 919: }
> 920: 
> 921: @SuppressWarnings("removal")

Could the scope be further reduced by applying it at line 926 in this method - 
at the cost of creating a temporary variable? The code could probably be 
further simplified by using a lambda...


PrivilegedAction pa = () -> new Socket(proxy);
@SuppressWarnings("removal")
var ps = AccessController.doPrivileged(pa);
s = ps;

-

PR: https://git.openjdk.java.net/jdk/pull/4138

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Daniel Fuchs]

On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang  wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  The essential change for this JEP, including the `@Deprecate` annotations 
>> and spec change. It also update the default value of the 
>> `java.security.manager` system property to "disallow", and necessary test 
>> change following this update.
>> 2. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  Automatic changes to other source files to avoid javac warnings on 
>> deprecation for removal
>> 
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
>> generated programmatically, see the comment below for more details. If you 
>> are only interested in a portion of the 3rd commit and would like to review 
>> it as a separate file, please comment here and I'll generate an individual 
>> webrev.
>> 
>> Due to the size of this PR, no attempt is made to update copyright years for 
>> any file to minimize unnecessary merge conflict.
>> 
>> Furthermore, since the default value of `java.security.manager` system 
>> property is now "disallow", most of the tests calling 
>> `System.setSecurityManager()` need to launched with 
>> `-Djava.security.manager=allow`. This is covered in a different PR at 
>> https://github.com/openjdk/jdk/pull/4071.
>> 
>> Update: the deprecation annotations and javadoc tags, build, compiler, 
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

src/java.base/share/classes/java/lang/SecurityManager.java line 104:

> 102:  * method will throw an {@code UnsupportedOperationException}). If the
> 103:  * {@systemProperty java.security.manager} system property is set to the
> 104:  * special token "{@code allow}", then a security manager will not be 
> set at

Can/should the `{@systemProperty ...}` tag be used more than once for a given 
system property? I thought it should be used only once, at the place where the 
system property is defined. Maybe @jonathan-gibbons can offer some more 
guidance on this.

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager

[Daniel Fuchs]

On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang  wrote:

> Please review the test changes for [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> With JEP 411 and the default value of `-Djava.security.manager` becoming 
> `disallow`, tests calling `System.setSecurityManager()`  need 
> `-Djava.security.manager=allow` when launched. This PR covers such changes 
> for tier1 to tier3 (except for the JCK tests).
> 
> To make it easier to focus your review on the tests in your area, this PR is 
> divided into multiple commits for different areas. Mostly the rule is the 
> same as how Skara adds labels, but there are several small tweaks:
> 
> 1. When a file is covered by multiple labels, only one is chosen. I make my 
> best to avoid putting too many tests into `core-libs`. If a file is covered 
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the 
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the 
> `rmi` commit.
> 4. One file not covered by any label -- 
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in 
> the `swing` commit.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> all files to minimize unnecessary merge conflict.
> 
> Please note that this PR can be integrated before the source changes for JEP 
> 411, as the possible values of this system property was already defined long 
> time ago in JDK 9.
> 
> Most of the change in this PR is a simple adding of 
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it 
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we 
> add the line.
> 
> There are several tests that launch another Java process that needs to call 
> the `System.setSecurityManager()` method, and the system property is added to 
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a 
> shell test).
> 
> 3 langtools tests are added into problem list due to 
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
> 
> 2 SQL tests are moved because they need different options on the `@run` line 
> but they are inside a directory that has a `TEST.properties`:
> 
> rename test/jdk/java/sql/{testng/test/sql/othervm => 
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi => 
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>  ```
> 
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.

I looked at serviceability, net, and corelibs. The changes look good to me - 
though I have one question about the NotificationEmissionTest. I believe that 
regardless of whether the new property is needed, test case 1 should be run in 
/othervm too.

test/jdk/javax/management/remote/mandatory/notif/NotificationEmissionTest.java 
line 34:

> 32:  * @run clean NotificationEmissionTest
> 33:  * @run build NotificationEmissionTest
> 34:  * @run main NotificationEmissionTest 1

This test case (NotificationEmissionTest 1) calls 
`System.setProperty("java.security.policy", policyFile);` - even though it 
doesn't call System.setSecurityManager(); Should the @run command line for test 
case 1 be modified as well?

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4071

Re: RFR: 8266567: Fix javadoc tag references in sun.management.jmxremote.ConnectorBootstrap [v4]

[Daniel Fuchs]

On Wed, 12 May 2021 09:14:53 GMT, Pavel Rappo  wrote:

>> This fixes two javadoc tag references and several typos. References are 
>> fixed by removing whitespace before the opening `(`. That whitespace caused 
>> the opening `(` and the rest of the reference to be parsed as a link label.
>> 
>> Since we are here, I think this class could also benefit from using modern 
>> APIs. This should be done in a separate PR though. For example:
>> 
>> 1. Numerous instances of `Boolean.valueOf(  ).booleanValue()` could be 
>> changed to `Boolean.parseBoolean(  )`
>> 2. `throw (IllegalArgumentException) new 
>> IllegalArgumentException(msg).initCause(e);` could be changed to `throw new 
>> IllegalArgumentException(msg, e);`
>
> Pavel Rappo has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added missing full stops

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3885

Re: RFR: 8266567: Fix javadoc tag references in sun.management.jmxremote.ConnectorBootstrap [v2]

[Daniel Fuchs]

On Wed, 5 May 2021 18:39:14 GMT, Pavel Rappo  wrote:

>> This fixes two javadoc tag references and several typos. References are 
>> fixed by removing whitespace before the opening `(`. That whitespace caused 
>> the opening `(` and the rest of the reference to be parsed as a link label.
>> 
>> Since we are here, I think this class could also benefit from using modern 
>> APIs. This should be done in a separate PR though. For example:
>> 
>> 1. Numerous instances of `Boolean.valueOf(  ).booleanValue()` could be 
>> changed to `Boolean.parseBoolean(  )`
>> 2. `throw (IllegalArgumentException) new 
>> IllegalArgumentException(msg).initCause(e);` could be changed to `throw new 
>> IllegalArgumentException(msg, e);`
>
> Pavel Rappo has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed more typos

src/jdk.management.agent/share/classes/sun/management/jmxremote/ConnectorBootstrap.java
 line 308:

> 306:   public static synchronized JMXConnectorServer initialize() {
> 307: 
> 308:  // Load new management properties

Debatable. I guess what was meant here is:

// Loads a new management Properties instance

Otherwise LGTM.

-

PR: https://git.openjdk.java.net/jdk/pull/3885

Re: RFR: 8266567: Fix javadoc tag references in sun.management.jmxremote.ConnectorBootstrap

[Daniel Fuchs]

On Wed, 5 May 2021 15:59:43 GMT, Pavel Rappo  wrote:

> This fixes two javadoc tag references and several typos. References are fixed 
> by removing whitespace before the opening `(`. That whitespace caused the 
> opening `(` and the rest of the reference to be parsed as a link label.
> 
> Since we are here, I think this class could also benefit from using modern 
> APIs. This should be done in a separate PR though. For example:
> 
> 1. Numerous instances of `Boolean.valueOf(  ).booleanValue()` could be 
> changed to `Boolean.parseBoolean(  )`
> 2. `throw (IllegalArgumentException) new 
> IllegalArgumentException(msg).initCause(e);` could be changed to `throw new 
> IllegalArgumentException(msg, e);`

LGTM. Thanks for fixing the various typos.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3885

Integrated: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records

[Daniel Fuchs]

On Thu, 25 Mar 2021 17:30:52 GMT, Daniel Fuchs  wrote:

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> 
> And the equivalent with a record class: 
> 
> 
> public record NamedNumber(int number, String name) {}

This pull request has now been integrated.

Changeset: d84a7e55
Author:Daniel Fuchs 
URL:   https://git.openjdk.java.net/jdk/commit/d84a7e55
Stats: 872 lines in 3 files changed: 838 ins; 12 del; 22 mod

8264124: Update MXBean specification and implementation to extend mapping of 
CompositeType to records

Reviewed-by: mchung, chegar, alanb

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v6]

[Daniel Fuchs]

On Wed, 31 Mar 2021 21:43:58 GMT, Mandy Chung  wrote:

>> Daniel Fuchs has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - minor style issue
>>  - minor style issue
>
> Thanks for making the change.  The spec change looks good to me.

@mlchung @AlanBateman @ChrisHegarty  would you formally approve the fix? The 
CSR has been approved.

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v7]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs 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 13 additional commits since the 
last revision:

 - Merge
 - minor style issue
 - minor style issue
 - Integrated review feedback
 - Improved test
 - Moved mapping for records just before Mapping for MXBean interface; Improved 
test.
 - Merge branch 'master' into mxbeans-8264124
 - Minor tweaks. Improved test.
 - Merge branch 'master' into mxbeans-8264124
 - Integrated review feedback. Updated the section for Mapping to records.
 - ... and 3 more: https://git.openjdk.java.net/jdk/compare/5fe7d4c4...257a6ed8

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/a624a763..257a6ed8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=05-06

  Stats: 5593 lines in 174 files changed: 3784 ins; 623 del; 1186 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v6]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs has updated the pull request incrementally with two additional 
commits since the last revision:

 - minor style issue
 - minor style issue

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/9227ba58..a624a763

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=04-05

  Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Daniel Fuchs]

On Wed, 31 Mar 2021 18:32:31 GMT, Mandy Chung  wrote:

>> Hi Mandy, I have updated the PR with a revised version of the text above. Is 
>> that more in line what you had in mind?
>> 
>> best regards,
>> -- daniel
>
> This looks better.  I like a separate mapping section for records since the 
> mapping rules are straight-forward.   The complexity comes if we want to 
> support a record to implement `CompositeDataView` that allows a type to 
> support more flexible conversion to `CompositeData` which is why you add 
> records in the "Mappings for other types".   I am wondering if we really want 
> to support records to implement `CompositeDataView` but we may want to avoid 
> such a special case.
> 
> I suggest add subsections in "Mappings for other types":   There are two 
> mapping rules: (1) _opentype(J)_ maps `J` to `CompositeType` (2) 
> _opendata(J)_ maps `J` to `CompositeData`.   _opentype(J)_ for record class 
> does not need to refer to other types.   The common cases for _opendata(J)_ 
> for record class is using the canonical constructors.   The other less common 
> cases like using non-canonical constructors and `CompositeDataView` can state 
> that it follows the same rules as specified for other types.   "Mappings for 
> other types" does not need any change for records.   
> 
> "Reconstructing an instance of Java type J from a CompositeData" section 
> should be renamed to "Reconstructing an instance of Java type or record class 
> J from a CompositeData"
> 
> Here is a minor tweaking on "Mappings for Records"
> 
> A record is convertible to a CompositeType if and only if all its components 
> are 
> convertible to open types. Otherwise, it is not convertible.
> 
> A record class is converted to a CompositeType with one item for every record 
> component as follows.
> - The type name of this CompositeType is determined by the type name rules 
> detailed below. 
> - Each record component of type T, the item in the CompositeType has the same 
> name 
>   as the record component and of type opentype(T).
> 
> The mapping from an instance of a record class to a CompositeData 
> corresponding to 
> the CompositeType is the same as specified as for other types (add a link).
> 
> A record is reconstructed from a CompositeData using its canonical 
> constructor. 
> The canonical constructor doesn't require the presence of 
> @javax.management.ConstructorParameters 
> or @java.beans.ConstructorProperties annotations. If these annotations are 
> present on
> the canonical constructor, they will be ignored.
> 
> If the CompositeData from which the record is reconstructed doesn't contain 
> all the record components, 
> the MXBean framework will attempt to reconstruct the record in the same way 
> as specified for 
> other types (add a link).

I have updated the PR with those changes, plus a few minor adjustments.

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v5]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs has updated the pull request incrementally with two additional 
commits since the last revision:

 - Integrated review feedback
 - Improved test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/7d478dec..9227ba58

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=03-04

  Stats: 114 lines in 2 files changed: 53 ins; 13 del; 48 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v4]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs has updated the pull request incrementally with one additional 
commit since the last revision:

  Moved mapping for records just before Mapping for MXBean interface; Improved 
test.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/59965133..7d478dec

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=02-03

  Stats: 127 lines in 2 files changed: 81 ins; 44 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Daniel Fuchs]

On Mon, 29 Mar 2021 22:12:01 GMT, Daniel Fuchs  wrote:

>> You add record in "Mappings for other types".  I think it deserves a 
>> separate section "Mappings for record classes" (maybe after primitive 
>> types).   It's useful to add a row for record in the summary table above 
>> "Mappings for primitive types"
>
> Link added. With respect to adding a section for records, it's a bit 
> difficult to separate that from the "Mapping for other types" without a lot 
> of duplication. I can add a row in the summary table, and then add a new 
> section "Mapping for records" just before "Mapping for other types", that 
> just gives a brief overview and refers to the mapping for other types below. 
> 
> Something like this - what do you think?
> 
> Mappings for Records
> 
> A {@linkplain Record record} R whose {@linkplain
>   Class#getRecordComponents() components} are
>   all convertible to open types, is itself convertible to a
>   {@link CompositeType} as follows.
>   The type name of this {@code CompositeType}
>   is determined by the same type name rules
>   defined by the Mapping for other types
>   below. Its getters are the accessors for the {@linkplain
>   RecordComponent record components}, and the record is reconstructed
>   using its canonical constructor, without needing any annotation.
> 
> A record may also expose additional non-canonical constructors, which
>   can be used to reconstruct the record if the composite data does
>   not exactly contain all the components expected by the
>   canonical constructor. However, in order to be taken into account
>   by the MXBean framework, such non-canonical constructors
>   need to be annotated with either the {@link ConstructorParameters
>   javax.management.ConstructorParameters} or
>  {@code @java.beans.ConstructorProperties} annotation.
> 
> The complete rules for the mapping are detailed as part
>   of the Mapping for other types
>   below.
> 
> Mappings for other types
> 
> Given a record, or a Java class or interface J that does not 
> match the other
>   rules in the table above, the MXBean framework will attempt to map
>   it to a {@link CompositeType} as follows.  []

Hi Mandy, I have updated the PR with a revised version of the text above. Is 
that more in line what you had in mind?

best regards,
-- daniel

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v3]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs 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 seven additional commits since 
the last revision:

 - Merge branch 'master' into mxbeans-8264124
 - Minor tweaks. Improved test.
 - Merge branch 'master' into mxbeans-8264124
 - Integrated review feedback. Updated the section for Mapping to records.
 - Integrated review feedback. Added a section for Mapping to records.
 - Fixed typo. Moved example with record after example with from method to 
respect the logical order of precedence.
 - 8264124: Update MXBean specification and implementation to extend mapping of 
CompositeType to records

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/1c3292ce..59965133

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=01-02

  Stats: 15001 lines in 575 files changed: 11278 ins; 1193 del; 2530 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Daniel Fuchs]

On Sat, 27 Mar 2021 01:49:38 GMT, Mandy Chung  wrote:

>> src/java.management/share/classes/javax/management/MXBean.java line 757:
>> 
>>> 755: 
>>> 756: If the class is a {@link Record}, its getters are the
>>> 757:   accessors for the record components. Otherwise, the
>> 
>> It may be good to add a link like {@linkplain RecordComponent record 
>> components}
>
> You add record in "Mappings for other types".  I think it deserves a separate 
> section "Mappings for record classes" (maybe after primitive types).   It's 
> useful to add a row for record in the summary table above "Mappings for 
> primitive types"

Link added. With respect to adding a section for records, it's a bit difficult 
to separate that from the "Mapping for other types" without a lot of 
duplication. I can add a row in the summary table, and then add a new section 
"Mapping for records" just before "Mapping for other types", that just gives a 
brief overview and refers to the mapping for other types below. 

Something like this - what do you think?

Mappings for Records

A {@linkplain Record record} R whose {@linkplain
  Class#getRecordComponents() components} are
  all convertible to open types, is itself convertible to a
  {@link CompositeType} as follows.
  The type name of this {@code CompositeType}
  is determined by the same type name rules
  defined by the Mapping for other types
  below. Its getters are the accessors for the {@linkplain
  RecordComponent record components}, and the record is reconstructed
  using its canonical constructor, without needing any annotation.

A record may also expose additional non-canonical constructors, which
  can be used to reconstruct the record if the composite data does
  not exactly contain all the components expected by the
  canonical constructor. However, in order to be taken into account
  by the MXBean framework, such non-canonical constructors
  need to be annotated with either the {@link ConstructorParameters
  javax.management.ConstructorParameters} or
 {@code @java.beans.ConstructorProperties} annotation.

The complete rules for the mapping are detailed as part
  of the Mapping for other types
  below.

Mappings for other types

Given a record, or a Java class or interface J that does not 
match the other
  rules in the table above, the MXBean framework will attempt to map
  it to a {@link CompositeType} as follows.  []

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Daniel Fuchs]

> This RFE proposes to extend the MXBean framework to define a mapping to 
> records.
> 
> The MXBean framework already defines a mapping of `CompositeType` to plain 
> java objects. Records are a natural representation of CompositeTypes. A 
> record can be easily reconstructed from a `CompositeData` through the record 
> canonical constructor. A clear advantage of records over plain java objects 
> is that the canonical constructor will not need to be annotated in order to 
> map composite data property names to constructor parameter names.
> 
> With this RFE, here is an example comparing coding a composite type 
> `NamedNumber` that consists of an `int` and a `String`, using records and 
> using a plain java class. In both case, the `CompositeType` looks like this:
> 
> CompositeType(
> "NamedNumber",  // typeName
> "NamedNumber",  // description
> new String[] {"number", "name"},// itemNames
> new String[] {"number", "name"},// itemDescriptions
> new OpenType[] {SimpleType.INTEGER,
> SimpleType.STRING}  // itemTypes
> );
> 
> The plain Java class needs a public constructor annotated with 
> `@ConstructorParameters` annotation:
> 
> public class NamedNumber {
> public int getNumber() {return number;}
> public String getName() {return name;}
> @ConstructorParameters({"number", "name"})
> public NamedNumber(int number, String name) {
> this.number = number;
> this.name = name;
> }
> private final int number;
> private final String name;
> }
> 
> And the equivalent with a record class: 
> 
> public record NamedNumber(int number, String name) {}

Daniel Fuchs has updated the pull request incrementally with one additional 
commit since the last revision:

  Fixed typo. Moved example with record after example with from method to 
respect the logical order of precedence.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3201/files
  - new: https://git.openjdk.java.net/jdk/pull/3201/files/bf8b437f..1c3292ce

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3201=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3201=00-01

  Stats: 19 lines in 1 file changed: 9 ins; 9 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Daniel Fuchs]

On Fri, 26 Mar 2021 14:45:05 GMT, Alan Bateman  wrote:

>> Daniel Fuchs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed typo. Moved example with record after example with from method to 
>> respect the logical order of precedence.
>
> src/java.management/share/classes/javax/management/MXBean.java line 792:
> 
>> 790:accessor for a record component {@code name} of type T,
>> 791:then the item in the {@code CompositeType} has the same name
>> 792:than the record component, and has type opentype(T).
> 
> The update to the MXBean spec looks good. Do you have a typo here, I assume 
> it should say "as the record component".

Right, thanks. Qualifying it as "a typo" was too kind ;-)
I have updated the PR. I also moved the `record` example after the `from` 
example to respect the order of precedence.

-

PR: https://git.openjdk.java.net/jdk/pull/3201

RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records

[Daniel Fuchs]

This RFE proposes to extend the MXBean framework to define a mapping to records.

The MXBean framework already defines a mapping of `CompositeType` to plain java 
objects. Records are a natural representation of CompositeTypes. A record can 
be easily reconstructed from a `CompositeData` through the record canonical 
constructor. A clear advantage of records over plain java objects is that the 
canonical constructor will not need to be annotated in order to map composite 
data property names to constructor parameter names.

With this RFE, here is an example comparing coding a composite type 
`NamedNumber` that consists of an `int` and a `String`, using records and using 
a plain java class. In both case, the `CompositeType` looks like this:

CompositeType(
"NamedNumber",  // typeName
"NamedNumber",  // description
new String[] {"number", "name"},// itemNames
new String[] {"number", "name"},// itemDescriptions
new OpenType[] {SimpleType.INTEGER,
SimpleType.STRING}  // itemTypes
);

The plain Java class needs a public constructor annotated with 
`@ConstructorParameters` annotation:

public class NamedNumber {
public int getNumber() {return number;}
public String getName() {return name;}
@ConstructorParameters({"number", "name"})
public NamedNumber(int number, String name) {
this.number = number;
this.name = name;
}
private final int number;
private final String name;
}

And the equivalent with a record class: 

public record NamedNumber(int number, String name) {}

-

Commit messages:
 - 8264124: Update MXBean specification and implementation to extend mapping of 
CompositeType to records

Changes: https://git.openjdk.java.net/jdk/pull/3201/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3201=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264124
  Stats: 679 lines in 3 files changed: 647 ins; 12 del; 20 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3201.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3201/head:pull/3201

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8263855: Use the blessed modifier order in java.management/naming [v2]

[Daniel Fuchs]

On Fri, 19 Mar 2021 17:13:58 GMT, Alex Blewitt 
 wrote:

>> As with #2993 changing the order of `final static` to `static final` for the 
>> `java.management`, `java.management.rmi` and `java.naming` modules.
>
> Alex Blewitt has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added more replacements of 'final public' with 'public final'

Marked as reviewed by dfuchs (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/3078

Re: RFR: 8262001: java/lang/management/ThreadMXBean/ResetPeakThreadCount.java failed with "RuntimeException: Current Peak = 14 Expected to be == previous peak = 7 + 8"

[Daniel Fuchs]

On Tue, 9 Mar 2021 01:31:33 GMT, Alex Menkov  wrote:

> The fix updates ResetPeakThreadCount.java test - increases number of threads, 
> but relaxes conditions so start/termination of system threads don't cause 
> failures.
> Additional changes:
> - store threads in a list instead of array (we need only to start/terminate 
> some number of threads, so linked list works better);
> - flags for thread termination are moved to thread class;
> - store values from ThreadMXBean.getPeakThreadCount() and getThreadCount() in 
> int instead of long (the methods return int values)

This looks like a good cleanup to me. Please wait for a review from someone 
from serviceability-dev before pushing.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2885

Re: RFR: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach [v2]

[Daniel Fuchs]

On Mon, 22 Feb 2021 21:36:52 GMT, Alex Menkov  wrote:

>> The fix also partially fixes JdwpAttachTest failures (JDK-8253940).
>> The failures are caused by network configuration changes during test 
>> execution ("global" IPv6 addresses disappears from interface).
>> To minimize chances of intermittent failures like this java.net tests use 
>> only link-local addresses whenever possible.
>> The fix does the same for JDI tests 
>> (Utils.getAddressesWithSymbolicAndNumericScopes  is used by JdwpListenTest 
>> and JdwpAttachTest)
>
> Alex Menkov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   include loopback addresses in testing

The last changes look good to me.

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2633

Re: RFR: 8224775: test/jdk/com/sun/jdi/JdwpListenTest.java failed to attach

[Daniel Fuchs]

On Thu, 18 Feb 2021 21:43:00 GMT, Alex Menkov  wrote:

> The fix also partially fixes JdwpAttachTest failures (JDK-8253940).
> The failures are caused by network configuration changes during test 
> execution ("global" IPv6 addresses disappears from interface).
> To minimize chances of intermittent failures like this java.net tests use 
> only link-local addresses whenever possible.
> The fix does the same for JDI tests 
> (Utils.getAddressesWithSymbolicAndNumericScopes  is used by JdwpListenTest 
> and JdwpAttachTest)

I don't see any specific issue with the proposed change but I don't know the 
JDWP tests enough to provide more feedback than that. Do you have special test 
cases for the IPv6 loopback? AFAIU this code here will filter it out?

-

PR: https://git.openjdk.java.net/jdk/pull/2633

Re: RFR: 8260448: Simplify ManagementFactory$PlatformMBeanFinder

[Daniel Fuchs]

On Tue, 26 Jan 2021 19:34:35 GMT, Claes Redestad  wrote:

> Simplify and desugar the static initialization of PlatformMBeanFinder. 
> 
> While arguably making the code easier to comprehend, this saves 5-6ms on 
> startup in various applications such as the spring-petclinic.

LGTM - it's a bit sad to transform lambda expressions back into for loops 
though ;-)

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2244

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Daniel Fuchs]

On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung  wrote:

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

Hi Mandy,

This looks good to me. There are a few places where a single call to 
`Reference::get` is replaced by multiple calls to `Reference::refersTo`, 
allowing the reference to get cleared in between, but as far as I could see 
that doesn't affect the overall logic which is still sound.
So LGTM!

best regards,
-- daniel

-

Marked as reviewed by dfuchs (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1609

Re: jmx-dev MBean attribute deprecation

[Daniel Fuchs]

Hi Milan,

JMX is being maintained by serviceability these days.
I am forwarding your question there.

This is an interesting observation. JMX makes it possible to
define and use annotations on methods/getters/setters, to add
key/value pairs to the Descriptor of the corresponding
MBeanFeatureInfo.

The mechanism should probably be extended so that the standard
@Deprecated annotation is also reflected in the Descriptor.

best regards,

-- daniel

On 23/07/2020 13:25, Milan Mimica wrote:

Hello list

With deprecation of OperatingSystem#SystemCpuLoad in favor of 
OperatingSystem#CpuLoad [1] an issue raised with generic tools that 
query all MBean attributes. Querying CPU usage is not side-effect-free: 
the immediate subsequent query for CPU load will often return a skewed 
value (on Linux at least, haven't checked others)[2]. This results in 
exported data where either new or deprecated MBean value is wrong and 
misleading.


There are many tools and libraries that generically export JMX metrics, 
so I think this should be addressed somehow. Maybe we could add a 
property in MBeanAttributeInfo which would mark the attribute as 
deprecated? These tools could then skip such metrics.



[1] 
https://docs.oracle.com/en/java/javase/14/docs/api/jdk.management/com/sun/management/OperatingSystemMXBean.html#getSystemCpuLoad()
[2] 
https://medium.com/@milan.mimica/the-java-cpu-usage-observer-effect-18808b18323f


--
Milan Mimica

Re: jmx-dev RFR 8240604: Rewrite sun/management/jmxremote/bootstrap/CustomLauncherTest.java test to make binaries from source file

[Daniel Fuchs]

Hi Alexander,

Fixes to JMX & management agent are reviewed on the
seviceability-dev (added in to:) these days.

best regards,

-- daniel

On 05/03/2020 13:17, Alexander Scherbatiy wrote:

Hello,

Could you review a small enhancement where the test CustomLauncherTest 
is updated to build binary launcher file from launcher.c file.
The file launcher.c is renamed to exelauncher.c to follow the name 
convention for executable test files building by jdk make system.


Bug: https://bugs.openjdk.java.net/browse/JDK-8240604
Webrev: http://cr.openjdk.java.net/~alexsch/8240604/webrev.00

The changes for obsolete binary files from 
sun/management/jmxremote/bootstrap/linux-* and solaris-* are not 
included into the webrev. They needs to be removed manually.


The test is passed on Ubuntu 18.04 x86-64, Solaris Sparc v9 11.2, and 
Solaris x64 11.4 systems.


The test is excluded from Windows and Mac Os X systems.

Thanks,
Alexander.

Re: RFR 8232622: Technical debt in BadAttributeValueExpException

[Daniel Fuchs]

Hi Roger,

OK - I can accept that then.

best regards,

-- daniel

On 13/02/2020 18:18, Roger Riggs wrote:

Hi Daniel,

That's part of the technical debt that has to go.

The change to do the conversion in the constructor has been there since 
JDK8.

And the value of the argument is informative.

There isn't too much overhead in keeping them but the are just noise at 
this point.


Thanks, Roger

Re: RFR 8232622: Technical debt in BadAttributeValueExpException

[Daniel Fuchs]

Hi Roger,

I think you will need to preserve these cases:

On 13/02/2020 15:52, Roger Riggs wrote:

-    || valObj instanceof Long
-    || valObj instanceof Integer
-    || valObj instanceof Float
-    || valObj instanceof Double
-    || valObj instanceof Byte
-    || valObj instanceof Short
-    || valObj instanceof Boolean) {


They could legitimately be transmitted by an older unpatched JVM.
we don't want to use System.identityHashCode(valObj) + "@" + 
valObj.getClass().getName(); for these.


best regards,

-- daniel

Re: jmx-dev RFR(S): 8234484: Add ability to configure third port for remote JMX

[Daniel Fuchs]

Thanks Felix!

On 16/01/2020 02:01, Yangfei (Felix) wrote:
Modified accordingly in new webrev:http://cr.openjdk.java.net/~fyang/8234484/webrev.02/   


Also renamed property to: com.sun.management.jmxremote.local.port


That looks fine to me.

best regards,

-- daniel

Re: RFR: 8236873: Worker has a deadlock bug

[Daniel Fuchs]

Hi Daniil,

That looks fine to me.

best regards,

-- daniel

On 15/01/2020 18:15, Daniil Titov wrote:

Please review a change [1] that fixes a deadlock issue [2] in 
sun.tools.jconsole.Worker class.

There is no need in guarding "stopped" flag by a lock. The fix removes this 
excessive locking and
instead makes the flag volatile.

Mach5 tier1-tier3 tests passed.

[1] Webrev : http://cr.openjdk.java.net/~dtitov/8236873/webrev.01/
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8236873

Best regards,
Daniil

Re: jmx-dev RFR: 8213222: remove RMIConnectorServer.CREDENTIAL_TYPES

[Daniel Fuchs]

Hi Daniil,

Looks good to me as well. I wonder however if the release note
should point to the replacement?
Something like:

The terminally deprecated constant 
`javax.management.remote.rmi.RMIConnectorServer.CREDENTIAL_TYPE` has 
been removed. A filter pattern can be specified instead using 
`RMIConnectorServer.CREDENTIALS_FILTER_PATTERN`.


best regards,

-- daniel


On 11/01/2020 04:52, Daniil Titov wrote:

Please review a change [1] that removes constant 
javax.management.remote.rmi.RMIConnectorServer.CREDENTIAL_TYPE.
This constant represents the name of the attribute that specifies a list of 
class names acceptable to the RMIServer.newClient()
remote method call. This constant was superseded by 
RMIConnectorServer.CREDENTIALS_FILTER_PATTERN and marked as
deprecated for removal in JDK 10 [3].

A new CRS [4]  and a release note sub-task [5]  were also created. Please 
review the CSR [4] as well.

Text search over the repository didn't find any other usage of this constant. 
Tier1 - tier3 Mach5 tests succeeded.

[1] Webrev:  http://cr.openjdk.java.net/~dtitov/8213222/webrev.01/
[2] Issue: https://bugs.openjdk.java.net/browse/JDK-8213222
[3] https://bugs.openjdk.java.net/browse/JDK-8191313
[4] CSR:  https://bugs.openjdk.java.net/browse/JDK-8236954
[5] Release Note sub-task: https://bugs.openjdk.java.net/browse/JDK-8236955

Thank you,
Daniil

Re: jmx-dev RFR(S): 8234484: Add ability to configure third port for remote JMX

[Daniel Fuchs]

Hi Felix,

On 11/01/2020 07:37, Yangfei (Felix) wrote:

Hi Daniel,

 Thanks for the suggestions.
 I modified the patch making the third port also configurable via the 
management.properties file.
 New webrev: http://cr.openjdk.java.net/~fyang/8234484/webrev.01/


I think I would prefer the try catch NumberFormatException
to be just around the call to Integer.parseInt(), as it is
done elsewhere in this file (e.g. line 337).



 Previously, we test the effectiveness of the patch by checking the port 
usage with -Dsun.rmi.transport.tcp.logLevel=VERBOSE.
 Is it good to modify the following test case specifying the third port for 
the testing purpose?


Please don't do that. That test is not appropriate for this - as
it opens a remote connector, not a local connector.

Also please get someone from serviceability-dev to also look at
changes (added in cc:).

best regards,

-- daniel



diff -r d630c0a63222 
test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
--- a/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java  
Wed Jan 08 08:53:28 2020 +0900
+++ b/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java  
Sat Jan 11 15:30:25 2020 +
@@ -31,6 +31,7 @@
  
  import jdk.test.lib.process.OutputAnalyzer;

  import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.Utils;
  
  /**

   * @test
@@ -203,6 +204,12 @@
  args.add("-Dcom.sun.management.jmxremote.host=" + address);
  args.add("-Dcom.sun.management.jmxremote.port=" + jmxPort);
  args.add("-Dcom.sun.management.jmxremote.rmi.port=" + rmiPort);
+try {
+int port = Utils.getFreePort();
+args.add("-Dcom.sun.management.jmxlocal.port=" + port);
+} catch (Exception e) {
+System.out.println(e);
+}
  args.add("-Dcom.sun.management.jmxremote.authenticate=false");
  args.add("-Dcom.sun.management.jmxremote.ssl=" + 
Boolean.toString(useSSL));
  // This is needed for testing on loopback

Felix



Hi Felix,

Shouldn't this be configurable via the management.properties file, like all 
other
JMX properties?

Also on the one hand - it would be good to have a test.
On the other hand - writing a stable test will be problematic as there is no way
to ensure that any particular port number will be available for the agent to 
bind
to.

best regards,

-- daniel

On 08/01/2020 01:09, Yangfei (Felix) wrote:

Hi,

Please review this patch adding a way to configure the third port for
remote JMX.

Currently, it is possible to configure two of the three ports for JMX
with com.sun.management.jmxremote.port and
com.sun.management.jmxremote.rmi.port.

However, there is no way to configure the third port. This can cause
problems if the randomly assigned port conflicts with another service
that has not yet acquired its port.

     Bug: https://bugs.openjdk.java.net/browse/JDK-8234484

    Webrev: http://cr.openjdk.java.net/~fyang/8234484/webrev.00/

    Passed tiered-1 test.

Thanks,

Felix

Re: RFR (M): 8231209: [REDO] ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread

[Daniel Fuchs]

Hi,

On 21/09/2019 00:28, Hohensee, Paul wrote:

java.lang.management.ThreadMXBean has two default methods that throw 
UnsupportedOperationException:

ThreadInfo[] getThreadInfo(long[], boolean, boolean, int)
ThreadInfo[] dumpAllThreads(boolean, boolean, int)

Without actually testing it, is it safe to assume that since 
com.sun.management.ThreadMXBean extends j.l.m.ThreadMXBean, the new 
c.s.m.ThreadMXBean method

 public default long getCurrentThreadAllocatedBytes() {
 throw new UnsupportedOperationException();
 }

will behave the same way?


Oh right, I'd forgotten about that. Looking at the history
it seems I'd even checked that it did work at the time. duh.

How time flies...

Thanks Paul!

best regards,

-- daniel

Re: RFR (M): 8231209: [REDO] ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread

[Daniel Fuchs]

Hi Paul,

It might be worth double checking what happens if you create a
MXBean proxy to access the com.sun.management.ThreadMXBean
in a remote server:

https://download.java.net/java/early_access/jdk14/docs/api/java.management/java/lang/management/ManagementFactory.html#getPlatformMXBean(javax.management.MBeanServerConnection,java.lang.Class)

and

https://download.java.net/java/early_access/jdk14/docs/api/java.management/javax/management/MBeanServerInvocationHandler.html#newProxyInstance(javax.management.MBeanServerConnection,javax.management.ObjectName,java.lang.Class,boolean)

are two different ways of doing this.

Will getCurrentThreadAllocatedBytes() return something
(however uninteresting that might be) or will the
default implementation be triggered on the client side and
UnsuportedOperationsException be thrown directly without
invoking the server?
Whatever the result is - it might be worth mentioning in the
CSR.

Best regards,

-- daniel

On 19/09/2019 17:39, Hohensee, Paul wrote:

Off by 2 error. Changed the subject to reflect 8231209.

http://cr.openjdk.java.net/~phh/8231209/webrev.00/

Paul

Re: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows

[Daniel Fuchs]

On 21/05/2019 04:25, Daniil Titov wrote:

Please review un updated version of the previous change that also removes 
unnecessary line

chmod ug+x $REVOKEALL

from test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh

Webrev:http://cr.openjdk.java.net/~dtitov/8214545/webrev.03  
Bug:https://bugs.openjdk.java.net/browse/JDK-8214545  


Looks good to me as well.

best regards,

-- daniel

Re: jmx-dev RFR: 8214545: sun/management/jmxremote/bootstrap tests hang in revokeall.exe on Windows

[Daniel Fuchs]

Hi,

On 20/05/2019 01:43, David Holmes wrote:

Webrev: http://cr.openjdk.java.net/~dtitov/8214545
Bug: https://bugs.openjdk.java.net/browse/JDK-8214545
The count-- is obvious as it is the loop counter, but it is far from 
clear to me that i++ is correct. I don't fully understand the logic but 
i is only incremented under very specific conditions. If you rewrote the 
code to avoid the use of the continue then i would not be modified 
except where it currently is.




Looks like `i` tries to count the entries that were not
modified - so the fact that it was not incremented before
`continue` looks like an oversight. I'd say that Daniil is
right.
I believe Alan wrote that tool - he may be able to confirm ;-)

That said - if we could do the same thing in java as Alan suggests
and replace these shell scripts with java that might be a big
win!

best regards,

-- daniel

Re: RFR: JDK-8224028: loop initial declarations introduced by JDK-8184770 (jdwp)

[Daniel Fuchs]

Hi Ao Qi,

I'm adding serviceability-dev, since this is for jdwp.

The proposed changes look good to me - but please get
someone from the serviceability team to review this.

best regards,

-- daniel


On 16/05/2019 08:41, Ao Qi wrote:

Hi,

I found build is failed on CentOS 7.6, because of loop initial
declarations. Could I please get reviews for this?

Bug:
https://bugs.openjdk.java.net/browse/JDK-8224028

Webrev:
http://cr.openjdk.java.net/~aoqi/8224028/webrev.00/

Tested:
linux-x86_64-server-release tier1

Thanks,
Ao Qi

Re: RFR: JDK-8184770: JDWP support for IPv6

[Daniel Fuchs]

Hi Arthur,

Not directly related to Alex's original question but...

On 30/03/2019 00:03, Arthur Eubanks wrote:
We have some ipv6 patches as well in this area as well (as well as other 
patches to support ipv6 only environments) that we're trying to 
upstream. I don't understand the code myself, but it might be useful to 
take a look:

http://cr.openjdk.java.net/~aeubanks/jdwpipv6/webrev.00/index.html


SocketTransportService.java:

On my machine at least, InetAddress.getByName("localhost") and
InetAddress.getLocalHost() are quite different:


InetAddress.getByName("localhost") will return the loopback (127.0.0.1)
InetAddress.getLocalHost() returns the local hostname (dhcp-XXX-XXX...)

If I'm not mistaken your changes in HostPort would transform something
that previously returned the loopback (no host specified) with something
that returns the local host name ("*" specified).


So I'm not sure these changes are quite right.
Maybe Alex will be able to confirm.

best regards,

-- daniel

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,

If you can update WAIT_FOR_JMX_AGENT_TIMEOUT_MS in 
JMXAgentInterfaceBinding.java

to 2, in you webrev.04, then I believe you should
be good to go and push the changes.

This seems to fix the instability I had observed with
fastdebug VMs.


best regards,

-- daniel

On 12/03/2019 18:22, Daniel Fuchs wrote:



This is what I have tried:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/04/webrev/

If you think we should try this one. That's fine with me.


I have now also tested webrev.04 with a test config that uses
the fastdebug VM and observed many intermittent failures.
The logs let me think that maybe the JMX agent needed to be
given more time to come up:

So I am now experimenting with your webrev.04 above - but with the
following additional change to JMXAgentInterfaceBinding.java:

     private static final int WAIT_FOR_JMX_AGENT_TIMEOUT_MS = 2;

So far the result are looking better, but tests are still running.
I wonder whether the same change would also reduce the number of
intermittent failures on your box?

best regards,

-- daniel

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,

On 08/03/2019 13:20, Severin Gehwolf wrote:

On Fri, 2019-03-08 at 11:59 +, Daniel Fuchs wrote:

- please verify on your local box if testing both plain &
SSL with a Thread.sleep(1000) in between makes the test
more stable. If it does, then I'd prefer we go down
this road rather than selecting one plain vs SSL at random.


I've tested this before. With a sleep of 1 or 2 seconds. It didn't work
reliably for me (it might have been a fastdebug JVM).


fastdebug VM (sigh!) - that's a config I hadn't tested.
see below...


Eventually, the
SSL version of the test would fail. Yet, the latest version of the
webrev used different sets of ports for SSL and plain so that led me to
believe it's not related to a port clash. So Thread.sleep() wouldn't
help. 


Good point.


Have you tried without the sleep at all and running both plain
and SSL in your test system?


Yes. As in your webrev.04 below - Works fine with regular VM.


This is what I have tried:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/04/webrev/

If you think we should try this one. That's fine with me.


I have now also tested webrev.04 with a test config that uses
the fastdebug VM and observed many intermittent failures.
The logs let me think that maybe the JMX agent needed to be
given more time to come up:

So I am now experimenting with your webrev.04 above - but with the
following additional change to JMXAgentInterfaceBinding.java:

private static final int WAIT_FOR_JMX_AGENT_TIMEOUT_MS = 2;

So far the result are looking better, but tests are still running.
I wonder whether the same change would also reduce the number of
intermittent failures on your box?

best regards,

-- daniel



Thanks,
Severin

Re: RFR: JDK-8220257: fix headings in java.instrument

[Daniel Fuchs]

On 12/03/2019 14:24, Gary Adams wrote:

Need one reviewer for this trivial correction .


Having had to do the same for java.logging, I can say that the
doc changes to package-info.java look good to me :-)

AFAICT the changes to the copyright notice look OK too.

best regards,

-- daniel



After pushing the GPL header update, the h3 to h2 tags will be updated.
The copyright will be changed with the headings, so I'll push that 
change first.
   JDK-8220474: Incorrect GPL header in 
src/java.instrument/share/classes/java/lang/instrument/package-info.java


diff --git 
a/src/java.instrument/share/classes/java/lang/instrument/package-info.java 
b/src/java.instrument/share/classes/java/lang/instrument/package-info.java
--- 
a/src/java.instrument/share/classes/java/lang/instrument/package-info.java
+++ 
b/src/java.instrument/share/classes/java/lang/instrument/package-info.java

@@ -1,11 +1,11 @@
  /*
- * Copyright (c) 2003, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2003, 2019, Oracle and/or its affiliates. All rights 
reserved.

   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
   *
   * This code is free software; you can redistribute it and/or modify it
   * under the terms of the GNU General Public License version 2 only, as
   * published by the Free Software Foundation.  Oracle designates this
- * particular file as subject to the Classpath exception as provided
+ * particular file as subject to the "Classpath" exception as provided
   * by Oracle in the LICENSE file that accompanied this code.
   *
   * This code is distributed in the hope that it will be useful, but 
WITHOUT

@@ -52,7 +52,7 @@
   *  Each of these ways to start an agent is described below.
   *
   *
- * Starting an Agent from the Command-Line Interface
+ * Starting an Agent from the Command-Line Interface
   *
   *  Where an implementation provides a means to start agents from the
   * command-line interface, an agent is started by adding the following 
option

@@ -113,7 +113,7 @@
   * threads, is legal from {@code premain}.
   *
   *
- * Starting an Agent After VM Startup
+ * Starting an Agent After VM Startup
   *
   *  An implementation may provide a mechanism to start agents 
sometime after

   * the the VM has started. The details as to how this is initiated are
@@ -164,7 +164,7 @@
   * by the JVM for troubleshooting purposes).
   *
   *
- * Including an Agent in an Executable JAR file
+ * Including an Agent in an Executable JAR file
   *
   *  The JAR File Specification defines manifest attributes for 
standalone

   * applications that are packaged as executable JAR files. If an
@@ -194,8 +194,8 @@
   * an uncaught exception or error, the JVM will abort.
   *
   *
- *  Loading agent classes and the modules/classes available to the 
agent

- * class 
+ *  Loading agent classes and the modules/classes available to the 
agent

+ * class 
   *
   *  Classes loaded from the agent JAR file are loaded by the
   * {@linkplain ClassLoader#getSystemClassLoader() system class loader} 
and are

@@ -242,7 +242,7 @@
   * In other words, a custom system class loader must support the 
mechanism to

   * add an agent JAR file to the system class loader search.
   *
- * Manifest Attributes
+ * Manifest Attributes
   *
   *  The following manifest attributes are defined for an agent JAR 
file:

   *
@@ -311,7 +311,7 @@
   * ignored).
   *
   *
- * Instrumenting code in modules
+ * Instrumenting code in modules
   *
   *  As an aid to agents that deploy supporting classes on the 
search path of
   * the bootstrap class loader, or the search path of the class loader 
that loads

@@ -323,4 +323,4 @@
   * @revised 9
   */

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,

On 05/03/2019 14:33, Severin Gehwolf wrote:

Hi Daniel,

Latest webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/03/webrev/

It's incorporating your changes wrt. to some timeouts and asserting the
expected exit code. Instead of the ProcessThread changes, I'm using the
sendMessage() approach. That API is already there.


Oh - nice finding, thanks for that! :-)


Unfortunately when running both SSL and plain sockets tests, it would
randomly fail for me. Even if I choose different sets of ports for
plain/ssl. So I've taken a different route of randomly selecting SSL or
plain. Overall, this should give reasonable coverage for both (plain
and SSL). This made test stability improve a lot on my Linux x86_64
machine. Ran for 100 iterations without failure.


Did you try it with a Thread.sleep(1000); statement inserted between
the plain test and the SSL test?

FWIW - I have imported your changes, and sent them as is in our
test system. Ran them 200 times on each platform - got only
one failure - on Linux x64, where one of the process failed with
"address already in use". I believe this is bound to happen from
time to time if by misfortune some other process on the host
is binding to the same port.

Then I modified your changes to test both plain and SSL, with
a sleep(1000) between the two - and observed the same behavior:
1 random failure out of 200 runs (which happened on Linux
x64 too but on a different machine in the farm).

I'd suggest to preemptively add the `@key intermittent` keyword
to the test - this hints to future maintainers (and to people that
analyze failures during build promotions) that the test is known
to fail intermittently.

1 out of 200 runs is probably an acceptable failure rate for that
kind of test. If I could think of any way to get rid of such random
failures I would insist that we fix the test before pushing it, but
unfortunately I have come to the conclusion that the nature of the
feature we're trying to test rules out any of the usual tricks we
could try.

So to sum it up:

- please add the `@key intermittent` keyword
- please verify on your local box if testing both plain &
  SSL with a Thread.sleep(1000) in between makes the test
  more stable. If it does, then I'd prefer we go down
  this road rather than selecting one plain vs SSL at random.

Then I think you're good to go - you can count me in as Reviewer.

If the failure rate becomes too noisy in the future, then we
can revisit - and one possibility will then be to turn this test
into a manual test.

Sorry to have been a bit picky - I hate intermittent failures
with a passion ;-)

best regards,

-- daniel



I'll run this through jdk/submit now. Could you run this through your
test system too, please?

Would you be OK with getting this patch pushed once we'd have positive
results for both?

Thanks,
Severin

On Fri, 2019-03-01 at 15:08 +, Daniel Fuchs wrote:

Hi Severin,

On 28/02/2019 15:47, Severin Gehwolf wrote:

Yes, thanks for looking at this Daniel. That was my determination as
well. However, even if we do all of the above, and add a test config
with /etc/hosts mapping a non-loopback address to "localhost" it would
break other tests. E.g. this one:
java/net/InetAddress/GetLocalHostWithSM.java

So I'd have to explore whether your suggestion with
InetAddress.getLocalHost() is viable. It seems promising.

I'll keep you posted.


Thanks for keeping the investigation going!

For what it's worth this is what I have been experimenting with:
http://cr.openjdk.java.net/~dfuchs/experiment-8219585/experiment.00/

It's only a POC and obviously need more cleaning.
Maybe some of the arbitrary timeouts in the test could be scaled
in accordance to timeout-factor (I think there's an adjustTimeout(long)
function somewhere in the test libs that does that).

I ran it 50 times in our test system - and it passed on all platforms,
so there's yet hope :-)

hope this helps,

-- daniel

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,

On 01/03/2019 15:49, Severin Gehwolf wrote:

On Fri, 2019-03-01 at 15:08 +, Daniel Fuchs wrote:

I ran it 50 times in our test system - and it passed on all platforms,
so there's yet hope :-)


Thanks for this! I take it, it actually runs the test on some of those
test systems (no trivial pass)?


Yes - AFAICT it ran on all systems - no trivial pass.
Used to fail on windows before I changed the parent/child
communication channel to use plain stdin instead of relying
on signals.

Note that I had to tweak the various timeouts to make it pass in
a seemingly reliable way - so there's still the risk that it
might start failing again in some esoteric configs...

best regards,

-- daniel



Cheers,
Severin

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,

On 28/02/2019 15:47, Severin Gehwolf wrote:

Yes, thanks for looking at this Daniel. That was my determination as
well. However, even if we do all of the above, and add a test config
with /etc/hosts mapping a non-loopback address to "localhost" it would
break other tests. E.g. this one:
java/net/InetAddress/GetLocalHostWithSM.java

So I'd have to explore whether your suggestion with
InetAddress.getLocalHost() is viable. It seems promising.

I'll keep you posted.


Thanks for keeping the investigation going!

For what it's worth this is what I have been experimenting with:
http://cr.openjdk.java.net/~dfuchs/experiment-8219585/experiment.00/

It's only a POC and obviously need more cleaning.
Maybe some of the arbitrary timeouts in the test could be scaled
in accordance to timeout-factor (I think there's an adjustTimeout(long)
function somewhere in the test libs that does that).

I ran it 50 times in our test system - and it passed on all platforms,
so there's yet hope :-)

hope this helps,

-- daniel

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,


On 27/02/2019 16:26, Daniel Fuchs wrote:

On Mac OS X and Linux - I saw it failed too - though with lower
frequency (like 3 or 4 times every 100 runs), and AFAICS
usually later and for a different reason: when it fails, it usually
timeouts when trying to test over SSL.

On Solaris - it seems to fail at least half of the times, usually
in timeout over SSL too.


My guess was wrong - and I might have found what caused most
of these intermittent failures on solaris/mac/linux: increasing
the time that the JMXAgentInterfaceBinding class waits for the
agent to come up (say, from 500 to 5000) and sleeping
for 1s between the plain socket test and the SSL test seems to
reduce drastically the frequency of failures.

The /timeout also needs to be bumped to allow for the extra time.

Remains only the issue with windows... I guess some alternate
communication channel could be used between the master process
and its subprocess? Like having the subprocess read on stdin,
waiting for the master process to tell it to exit for instance?

best regards

-- daniel

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

On 26/02/2019 18:58, Severin Gehwolf wrote:

FWIW, I use 'make test TEST="..."'. Isn't the testing system using that
too?


I am not sure. But I'd expect it to be similar.

[...]


When I was working on that patch years ago, I think one testing machine
got configured in a way so that it wouldn't trivially pass. That may
have changed at some point in time?


Probably.


WRT using the loopback as an alternate configuration,
I wonder - should we add the loopback address before
checking size() < 1?
Not sure how stable the test will be in that case.


It wouldn't help anything. In that case it wouldn't be a regression
test anymore as it would pass *prior* JDK-6425769 *and* after. With a
non-loopback address added too, it would fail prior and pass after.


Fair enough.


Also should we use InetAddress.getLoopbackAddress() instead
of using "127.0.0.1" directly?


InetAddress.getLoopbackAddress() is less deterministic than using IPv4
address. Interestingly the javadoc mentions it may return any loopback
address, not just 127.0.0.1 or ::1. Not sure whether that's a problem
or not, I'd be fine to change that if you prefer.


Not specially - if you believe the test would be less deterministic
that way. The only reason I asked was in the hope to avoid having a hard
dependency on IPv4 vs IPv6 buried in the test code.

So far I think your changes look good.
I am satisfied that they don't trigger intermittent failure,
and I believe these changes are a step in the right direction,
even if the test still passes trivially most of the time.

On a side note: to your knowledge is there anything that would
prevent the test to work meaningfully with the pair:
InetAddress.getLocalHost(), InetAdress.getLoopbackAddress(), after
verifying that InetAddress.getLocalHost().isLoopbackAddress() == false?

I experimented with that - but then the test failed on all
windows machines.
The subprocess seemed to succeed - I could see all the expected
traces saying that the JMX connection worked, but then
ProcessThread.stopProcess() seemed to cause the subprocess to
exit with 1 (instead of the expected 143).
Seems to happen 100% of the times on windows.

On Mac OS X and Linux - I saw it failed too - though with lower
frequency (like 3 or 4 times every 100 runs), and AFAICS
usually later and for a different reason: when it fails, it usually
timeouts when trying to test over SSL.

On Solaris - it seems to fail at least half of the times, usually
in timeout over SSL too.

My guess is that these lower frequency failures might be caused
by reusing the same ports again. We had intermittent test failures
in other networking tests that disappeared once we started binding
explicitly to the loopback address, and letting the system allocate
an ephemeral port (binding with port 0)

That makes me think that the test might have had stability issues if
by misfortune it did not trivially pass.

Maybe this test should really be made manual - @run main/manual [1]?
[1] https://openjdk.java.net/jtreg/tag-spec.html

What are your thoughts?

best regards,

-- daniel


Thanks,
Severin

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,

On 25/02/2019 15:31, Severin Gehwolf wrote:

Hi Daniel,

Thanks for the review!

Latest webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/webrev/


[...]

I'm not sure we need a larger timeout. It's 20 seconds on my machine
which seems more than enough. What makes you think we'd need to get
this increased and it would actually do anything for test stability?


Meanwhile I launched your proposed changes through our
test system. The good news is that there was no failure
(even when repeating the test 25 times), and without
changing the 5 timeout - so that may be OK.
Maybe my machine has something that slows it down.
Or Maybe I should have used a timeout factor of 4.
It does pass if I use -timeout:4 when invoking it with jtreg.

The not so good news is that it seems none of our test
machine has a non-loopback address configured for local host.
So even with your changes, the test always trivially pass.

I see the message says "Ignoring manual test" - but the
test is not manual?

[...]


Yes. I've tried to avoid those issues by allowing one non-loopback
address and 127.0.0.1 explicitly. Is that a reasonable assumption?



WRT using the loopback as an alternate configuration,
I wonder - should we add the loopback address before
checking size() < 1?
Not sure how stable the test will be in that case.
Also should we use InetAddress.getLoopbackAddress() instead
of using "127.0.0.1" directly?



My bet is on it passing trivially with:

"Ignoring manual test since no more than one IPs are configured for 'localhost'"



Well - yes - I believe that's what's happening.

best regards,

-- daniel

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,

Thanks for looking into this!

On 25/02/2019 15:31, Severin Gehwolf wrote:

Hi Daniel,

Thanks for the review!

Latest webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/webrev/

On Fri, 2019-02-22 at 18:31 +, Daniel Fuchs wrote:

Hi Severin,

Did you manage to make the test pass locally?


Yes. Here is the latest run:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/JMXInterfaceBindingTest.jtr

FWIW, it passed locally with the v1 change proposed. But then again, it
didn't do what it was intended for: Try to bind to multiple host:port
pairs on a single system. This didn't work prior JDK-6425769 as it
would bind to *all* interfaces. The updated webrev tries to remediate
this.


Thanks - I experimented with similar changes too.


This test has had a long history of failing, and I've come
to suspect that it might be flawed.


If it has a history of failing, why aren't there any bugs for it? How
did it fail? Are you referring to JDK-8145982 and JDK-8146015?


Yes - well agreed - long history might have been a bit exaggerated.


First - it has /timeout=5. I believe that's much too short.
It immediately failed the first time I ran it locally on my machine.


I'm not sure we need a larger timeout. It's 20 seconds on my machine
which seems more than enough. What makes you think we'd need to get
this increased and it would actually do anything for test stability?


Experience - and the fact that I saw it fail in timeout on my
local machine (albeit intermittently).
The test system runs tests with many different configurations,
some of them with various combination like -Xcomp or -Xint
which tend to have some measurable effects on JVM startup.
Tests that do compilation or start sub-processes are often
more sensitive to these option sets.
Usually, I would recommend not setting the timeout at all -
unless proven that the default jtreg timeout is too short.
Especially for those cases where the test is not supposed to
timeout.


Then it uses well known ports. That's bad. There's no guarantee
that these ports will be free.


Fair enough. Fixed in the latest webrev. Now picks a random port in
range [9100, 9200].


Thanks.


Then - it only use addresses configured for "localhost".

Can there ever be more than one such address?


Yes, a multi-homed computer could use this for example this in
/etc/hosts on Linux:

192.168.1.18 localhost
192.168.122.1 localhost


OK - thanks. Not sure how much stable that would be. Ideally we
should use port 0 - but then retrieving the actual port might
not be easy. Let's wait until we see the test fail.


Looks like this on my system:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8219585/02/JMXInterfaceBindingTest-multihome.jtr


If there is more
than one, I suspect the test will fail: I believe the subprocess
started by the test would need the additional
 -Djava.rmi.server.hostname=


That's needed for 127.0.0.1 (loopback) support, so I've added it. Other
IP addresses don't seem to need it. So the test does not need it per
se, but it's useful so that more configs can actually test this.


on the command line.
And even with that - it might still fail if those two addresses
can't be bound simultaneously on the same port.


No. Socket addresses are unique per host:port pair. If that wasn't
possible, the whole test would be pointless.


Network tests that use "localhost" and listen to 0.0.0.0 are
often more prone to intermittent failures. I think this is due
to the policy of reusing addresses and ports of the
underlying OS. This test doesn't bind to the wildcard address
however - so we can hope it will not fall into this category.



Then why are loopback addresses filtered out? Because of two
things I guess:

 - if several loopback are defined, I'm not sure you can
   bind them on the same port (and it might be difficult to
   figure that out)

 - if you bind to the loopback address, then you most probably
   need to start the subprocess with
-Djava.rmi.server.hostname=


Yes. I've tried to avoid those issues by allowing one non-loopback
address and 127.0.0.1 explicitly. Is that a reasonable assumption?


I believe so.


If we don't see this test failing, I suspect this is because
it is either never selected, or always passes trivially.
If we start selecting it - or have a configuration where it
doesn't trivially pass, I suspect it will fail.


My bet is on it passing trivially with:

"Ignoring manual test since no more than one IPs are configured for 'localhost'"


Well - I'm going to send your patch through our internal
test system - I'll let you know of the result.

best regards,

-- daniel




Thanks,
Severin




On 22/02/2019 15:04, Severin Gehwolf wrote:

Hi!

Bug: https://bugs.openjdk.java.net/browse/JDK-8219585

Could somebody please review this trivial testbug. For a config like
[1] the logic prior JDK-8145982 returned this list for
getAddressesForLocalHost():

[localhost/127

Re: RFR: 8219585: [TESTBUG] sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java passes trivially when it shouldn't

[Daniel Fuchs]

Hi Severin,

Did you manage to make the test pass locally?

This test has had a long history of failing, and I've come
to suspect that it might be flawed.

First - it has /timeout=5. I believe that's much too short.
It immediately failed the first time I ran it locally on my machine.

Then it uses well known ports. That's bad. There's no guarantee
that these ports will be free.

Then - it only use addresses configured for "localhost".

Can there ever be more than one such address? If there is more
than one, I suspect the test will fail: I believe the subprocess
started by the test would need the additional
   -Djava.rmi.server.hostname=
on the command line.
And even with that - it might still fail if those two addresses
can't be bound simultaneously on the same port.

Then why are loopback addresses filtered out? Because of two
things I guess:

   - if several loopback are defined, I'm not sure you can
 bind them on the same port (and it might be difficult to
 figure that out)

   - if you bind to the loopback address, then you most probably
 need to start the subprocess with
  -Djava.rmi.server.hostname=

If we don't see this test failing, I suspect this is because
it is either never selected, or always passes trivially.
If we start selecting it - or have a configuration where it
doesn't trivially pass, I suspect it will fail.

best regards,

-- daniel




On 22/02/2019 15:04, Severin Gehwolf wrote:

Hi!

Bug: https://bugs.openjdk.java.net/browse/JDK-8219585

Could somebody please review this trivial testbug. For a config like
[1] the logic prior JDK-8145982 returned this list for
getAddressesForLocalHost():

[localhost/127.0.0.1, localhost/192.168.1.18, localhost/0:0:0:0:0:0:0:1]

Post JDK-8145982, getAddressesForLocalHost() returns:

[localhost/192.168.1.18]

The fix is trivial. Just adjust the condition for as to when the test
should actually trivially pass:

diff --git 
a/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java 
b/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
--- a/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
+++ b/test/jdk/sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
@@ -176,8 +176,8 @@
  }
  
  public static void main(String[] args) {

-List addrs = getAddressesForLocalHost();
-if (addrs.size() < 2) {
+List addrs = getNonLoopbackAddressesForLocalHost();
+if (addrs.size() < 1) {
  System.out.println("Ignoring manual test since no more than one IPs 
are configured for 'localhost'");
  return;
  }
@@ -186,7 +186,7 @@
  System.out.println("All tests PASSED.");
  }
  
-private static List getAddressesForLocalHost() {

+private static List getNonLoopbackAddressesForLocalHost() {
  
  try {

  return NetworkInterface.networkInterfaces()


Testing: Manual testing on local setup. jdk/submit (currently running)

Thanks,
Severin


[1] $ grep localhost /etc/hosts | grep -v '::'
127.0.0.1localhost localhost.localdomain localhost4 localhost4.localdomain4
192.168.1.18 localhost

Re: [RFR]8215622: Add dump to file support for jmap histo

[Daniel Fuchs]

Hi Paul,

On 12/02/2019 16:27, Hohensee, Paul wrote:

I don't see a way to change the Status from Closed to anything else. There's no 
option I can find to do so.


It may be that only the assignee can see this.
On the closed CSRs assigned to me I can see a "Back to Draft" button
next to "More".

Hope this helps,

-- daniel

Re: Review Request JDK-8212795: ThreadInfoCompositeData.toCompositeData fails to map ThreadInfo to CompositeData

[Daniel Fuchs]

Hi Mandy,

I agree that this looks more robust and will be better for
long term maintainability. I'm just surprised that

 156 static CompositeType compositeType() {
 157 return STACK_TRACE_ELEMENT_COMPOSITE_TYPE;
 158 }

is no longer (or was never) needed in StackTraceElementCompositeData
when

 146 static CompositeType v5CompositeType() {
 147 return V5_COMPOSITE_TYPE;
 148 }

appears to still be needed.

Otherwise, this looks good to me.

best regards,

-- daniel

On 24/10/2018 23:53, Mandy Chung wrote:

This patch fixes the regression introduced by JDK-8198253 in 11.
It turns out that NetBeans uses the internal sun.management API to
convert ThreadInfo to CompositeData for performance reason.
ThreadInfoCompositeData::toCompositeData is no longer used
in JDK since JMX added the MXBean support in JDK 6. The fix for
JDK-8212197 resolves one issue reported [1] but not the bug in
ThreadInfoCompositeData::toCompositeData. Sven has filed an
issue in NetBeans to replace the use of JDK internal API.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8212795/webrev.00/

Thanks
Mandy
[1] 
http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-October/025512.html

[2] https://issues.apache.org/jira/browse/NETBEANS-1478

Re: RFR JDK-8212197: OpenDataException thrown when constructing CompositeData for StackTraceElement

[Daniel Fuchs]

Hi Mandy,

This looks good to me.

best regards,

-- daniel

On 15/10/2018 23:02, Mandy Chung wrote:

Webrev:
http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8212197/webrev.00/

This simple patch fixes the issue Sven reports [1] where
the order of names and values when creating CompositeData
for StackTraceElement is mismatched.  I keep names/values
in the order of the attributes defined from old to new
version.

Mandy
[1] 
http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-October/025512.html

Re: RFR (S) 8048215: [TESTBUG] java/lang/management/ManagementFactory/ThreadMXBeanProxy.java Expected non-null LockInfo

[Daniel Fuchs]

Hi David,

Good finding! Looks reasonable to me.

best regards,

-- daniel

On 15/10/2018 05:23, David Holmes wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8048215
webrev: http://cr.openjdk.java.net/~dholmes/8048215/webrev/

Simple race condition in the test. The main thread does checks that are 
only valid once the target thread has called o.wait() but there's 
nothing to ensure that point of execution is reached. The failure is 
easily reprodcued by just putting in a sleep after:


Object o = new Object();

Fix is to add a shared 'waiter' Object that the target waits upon and 
for which the main also synchronizes on such that the main thread can't 
proceed until wait() has been called and released the monitor.


Thanks,
David

Re: RFR : JDK-8170299 - Debugger does not stop inside the low memory notifications code

[Daniel Fuchs]

Hi Harsha,

On a high level point of view, I think the fix looks good.
I like the use of CopyOnWriteArrayList and removeIf.
IIUC there is a single instance of ServiceThread in the VM, so
using a static single thread executor to call the listeners
should preserve the order in which the notifications are handled
without putting any additional strain on the VM - since you're
adding a single thread, which is lazily started when the
first listener needs to be invoked.

I see in the history of serviceabilty-dev that David was expressing
some concerns about the change of behaviour that offloading the
invocation of the listener to another thread would cause.

On the one hand I agree with that assertion: the new single thread
in which the listener are executed probably has a different
ACC etc... than the ServiceThread, and offloading to another
thread also could potentially invalidate assumptions that the
listeners code might have made. So this might not be transparent
to applications/libraries who might be using these notifications.
Also, unfortunately, would creating the new executor/thread
(+ runnable) become an issue when it happens at the time where
the memory pressure is already high (i.e. when the low memory
threshold is crossed)?

I'm not expert enough in the JVM to know whether there's any
JDK internal code (JFR?) that make use of these notification
listeners, and which might get tripped by this new threading.

On the other hand, offloading the invocation of the listener to
a dedicated thread could as well be safer, as previously a
badly implemented listener might have wedged the service
thread (which I assume would have been be bad). With your fix
it will only wedge the

So all in all - maybe this is worth fixing but better early
in the release than late. I also wonder whether such a behavior
change should deserve a release note (or a switch to revert
to the old behavior - though I do hope that isn't necessary).

Hopefully David & Mandy will chime in :-)


Now some small comments:

  79 boolean isPresent = listenerList.stream().anyMatch(li 
-> li.listener.equals(listener));


=> we're losing the atomicity here as a new listener might just
   have been added in between the call to listenerList.remove and
   this line, but I'm not sure it's important enough to fix.
   IMHO this new impl is good enough - even if it doesn't behave
   exactly as the old.


 102 NotificationListener listener;
 103 NotificationFilter filter;
 104 Object handback;

=> If I'm not mistaken these could (and therefore should) be final.

 134 if (filter != null ? !filter.equals(that.filter) : 
that.filter != null) return false;
 135 return handback != null ? 
handback.equals(that.handback) : that.handback == null;


=> Using Objects.equals(o1,o2) would make the code more readable here.

 141 result = 31 * result + (filter != null ? 
filter.hashCode() : 0);
 142 result = 31 * result + (handback != null ? 
handback.hashCode() : 0);


=> Similar to above Objects.hashCode(obj) would make the code
   simpler.

 149 Thread thread = InnocuousThread.newThread(runnable);

=> on a previous round of comment on this list Mandy
   had suggested using InnocuousThread.newSystemThread(runnable);

   I believe the main difference is in the TCCL, which is null
   for this latter case. Do we know what the TCCL of the
   ServiceThread is?

 151 thread.setName("JMX Notification thread");

=> I would suggest "Platform MXBean Notification thread" - "JMX"
   is too general.

 156 private List listenerList = new 
CopyOnWriteArrayList<>();


=> should be private *final*


hope this helps,

-- daniel

On 17/07/2018 07:23, Harsha Wardhana B wrote:

Hi All,

Please review the fix for the bug,

JDK-8170299 - Debugger does not stop inside the low memory notifications 
code


webrev at,

http://cr.openjdk.java.net/~hb/8170299/webrev.00/

Description of the fix:

The debugger does not stop inside the listeners registered for 
notification from


1. com.sun.management.GarbageCollectorMXBean 2. sun.management.MemoryImpl 
(MemoryMXBean)
3. com.sun.management.DiagnosticCommandMBean

The listeners registered for above MBeans are invoked by 'ServiceThread' which 
is a hidden thread and is not visible to the debugger.

This issue was was already worked on before and below is the review thread for 
the same.

http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-September/021782.html
http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-December/022611.html

With the current fix, all the user registered callbacks for above MBeans 
are executed in a newly created SingleThreadExecutor. The above file is 
also re-factored to use CopyOnWriteArrayList for managing the listeners.


The fix has been tested in Mach5 by running all the tests under 
open/:jdk_management and closed/:jdk_management. 

Re: [11] RFR: 8205653: test/jdk/sun/management/jmxremote/bootstrap/RmiRegistrySslTest.java and RmiSslBootstrapTest.sh fail with handshake_failure

[Daniel Fuchs]

[ccing serviceability-dev@openjdk.java.net]

Hi Siba,

This looks good to me - but I'm not a SSL expert.
It would be good to get someone from the security team eyeball those
changes (Xuelei? Brad?)

I added serviceability-dev@openjdk.java.net in cc as this is where
reviews for JMX/Monitoring changes happen these days...

best regards,

-- daniel

On 28/06/2018 17:10, Sibabrata Sahoo wrote:

Hi,

Please review the patch for,

JBS: https://bugs.openjdk.java.net/browse/JDK-8205653

Webrev: http://cr.openjdk.java.net/~ssahoo/8205653/webrev.00/

Change:

The Test has been upgraded to address the following 2 cases,

 1. Add protocol support for TLSv1.3. The change is done in the config
file named “management_ssltest11_ok.properties.in”.
 2. Add support for legacy TLS. Now a new config file
“management_ssltest15_ok.properties.in” hold TLS protocol
“TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA” instead
of “SSL_RSA_WITH_RC4_128_SHA,SSL_RSA_WITH_RC4_128_MD5”.

Previously the Test was using DSA keys which is not compatible with 
TLSv1.3. So the keys has been upgraded to use RSA(2048 bit). Hence the 
instruction in “Readme.txt” changed which generates RSA(2048 bit) keys.


NOTE: Few Test was problem listed which are removed from the list now. 
Mach 5 result PASS with multiple try for all 14 Test belongs to 
“open/test/jdk/sun/management/jmxremote” folder.


Thanks,

Siba

Re: RFR JDK-8198253: ThreadInfo.from(CompositeData) assigning fields incorrectly in JDK 9

[Daniel Fuchs]

On 28/02/18 18:08, mandy chung wrote:

Hi Daniel,

The from method validates the CompositeType regardless of the attribute 
value is null or not. In addition, the CompositeData also  I updated the 
test to make sure that's the case.


I tweak the wording s/composite data/composite type/ to avoid the ambiguity.
  
  * The {@code "lockedStackFrame"} attribute in

  * {@link MonitorInfo#from(CompositeData) MonitorInfo}'s composite type
  * must represent {@code StackTraceElement} of the same version N.


Looks fine!

-- daniel



Mandy

On 2/28/18 3:07 AM, Daniel Fuchs wrote:

Hi Mandy,

This looks very good.

In the API documentation of ThreadInfo::from, below the table
that lists the attributes of StackTraceElement, I wonder if the
following text should be added for completeness:

```A CompositeData representing a MonitorInfo of version N must 
contain a lockedStackTrace attribute that is either null if 
stackDepth  < 0, or is a CompositeData representing a 
StackTraceElement of version N.```


What do you think?

best regards,

-- danie

On 27/02/2018 20:31, mandy chung wrote:
I made further edits to the javadoc and I am happy with this version 
(move out the attributes for StackTraceElement as a separate table).


Specdiff:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/specdiff/overview-summary.html 



javadoc:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/api/java/lang/management/ 



Webrev:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/webrev.02/

Mandy

On 2/27/18 10:55 AM, mandy chung wrote:

Good point, Jeremy.  I notice some strange-ness when I wrote it but
wasn't able to pin point the error.  Daniel also suggests to clarify
MonitorInfo as well.

Does this version look better?

  * Returns a {@code ThreadInfo} object represented by the
  * given {@code CompositeData}.
  * 
  * A {@code CompositeData} representing a {@code ThreadInfo} of
  * version N must contain all of the attributes defined
  * in version  N unless specified otherwise.
  * Same rule applies transitively to attributes whose type or
  * component type is {@code CompositeType}.
  * 
  * A {@code CompositeData} representing {@code ThreadInfo} of 
version

  * N contains {@code "stackTrace"} attribute representing
  * an array of {@code StackTraceElement} of version N.
  * The {@code "lockedMonitors"} attribute represents
  * an array of {@link MonitorInfo} of version N
  * which implies that its {@code "lockedStackFrame"} attribute 
also
  * represents {@code StackTraceElement} of the same version, 
N.
  * Otherwise, this method will throw {@code 
IllegalArgumentException}.


Mandy

Re: RFR JDK-8198253: ThreadInfo.from(CompositeData) assigning fields incorrectly in JDK 9

[Daniel Fuchs]

Hi Mandy,

This looks very good.

In the API documentation of ThreadInfo::from, below the table
that lists the attributes of StackTraceElement, I wonder if the
following text should be added for completeness:

```A CompositeData representing a MonitorInfo of version N must contain 
a lockedStackTrace attribute that is either null if stackDepth  < 0, or 
is a CompositeData representing a StackTraceElement of version N.```


What do you think?

best regards,

-- danie

On 27/02/2018 20:31, mandy chung wrote:
I made further edits to the javadoc and I am happy with this version 
(move out the attributes for StackTraceElement as a separate table).


Specdiff:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/specdiff/overview-summary.html

javadoc:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/api/java/lang/management/

Webrev:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/webrev.02/

Mandy

On 2/27/18 10:55 AM, mandy chung wrote:

Good point, Jeremy.  I notice some strange-ness when I wrote it but
wasn't able to pin point the error.  Daniel also suggests to clarify
MonitorInfo as well.

Does this version look better?
  


  * Returns a {@code ThreadInfo} object represented by the
  * given {@code CompositeData}.
  * 
  * A {@code CompositeData} representing a {@code ThreadInfo} of
  * version N must contain all of the attributes defined
  * in version  N unless specified otherwise.
  * Same rule applies transitively to attributes whose type or
  * component type is {@code CompositeType}.
  * 
  * A {@code CompositeData} representing {@code ThreadInfo} of version
  * N contains {@code "stackTrace"} attribute representing
  * an array of {@code StackTraceElement} of version N.
  * The {@code "lockedMonitors"} attribute represents
  * an array of {@link MonitorInfo} of version N
  * which implies that its {@code "lockedStackFrame"} attribute also
  * represents {@code StackTraceElement} of the same version, N.
  * Otherwise, this method will throw {@code IllegalArgumentException}.

Mandy

Re: RFR JDK-8198253: ThreadInfo.from(CompositeData) assigning fields incorrectly in JDK 9

[Daniel Fuchs]

Hi Mandy,

Nice to see this fixed.

In ThreadInfoCompositeData::initV6CompositeType

 425 if (name.equals(STACK_TRACE)) {
 426 ot = new ArrayType<>(1, 
StackTraceElementCompositeData.v5CompositeType());

 427 } else if (name.equals(LOCKED_MONITORS)) {
 428 ot = new ArrayType<>(1, 
MonitorInfoCompositeData.v5CompositeType());


This is probably OK for StackTraceElementCompositeData, but
is confusing for MonitorInfoCompositeData. I thought
MonitorInfo was added in 6? so why would it have a
v5CompositeType?

I'd suggest renaming v5 to v6 in MonitorInfoCompositeData, and also
adding a comment when calling 
StackTraceElementCompositeData.v5CompositeType()

to say that nothing changed in StackTraceElementCompositeData
between 6 and 5.

best regards,

-- daniel



On 24/02/2018 00:50, mandy chung wrote:

Webrev at:
   http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/webrev.01

This patch updates the spec to clarify what attributes are required
since which release.  There is a spec bug that "classLoaderName"
added in JDK 9 is missing in the CompositeData for StackTraceElement
but the implementation is correct.  I will file a CSR for this update.

This patch ensures that CompositeData for ThreadInfo of version N
must have the attributes defined since <= N.
ThreadInfo::from also makes sure 'stackTrace' and 'lockedMonitors'
attributes of version N while it can include additional attributes
which has been the current behavior.

JDK-8139587 intended to support older versions of StackTraceElement
which does not seem a complete solution.  I reverted the fix for
JDK-8139587 (mostly) and removed TypeVersionMapper.  The fix constructs
the CompositeType for ThreadInfo and MonitorInfo of different
versions and used them for validation.  Minor cleanup: the static
final variables are renamed to all capitals.  CompatibilityTest.java
test is missing the copyright header.

thanks
Mandy
[1]http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198253/api/java/lang/management/ThreadInfo.html#from(javax.management.openmbean.CompositeData)

Re: RFR JDK-8198253: ThreadInfo.from(CompositeData) assigning fields incorrectly in JDK 9

[Daniel Fuchs]

Hi Jeremy,

I'm not sure I understand exactly what is going on there.
I wonder if this has anything to do with an issue I noticed some
time back when the jvisualvm for JDK 8 and JDK 9 were still out
of sync:
https://bugs.openjdk.java.net/browse/JDK-8167099

I stumbled on that because at the time I was hacking my
developer's build of JDK 9 and trying to make it generate
JDK 8 CompositeDatas. But then as far as I could see that
bug (JDK-8167099) could not be observed in a regular
environment - it was just tickled by my hacks.

Now I'm wondering if what you're seeing is more or less
related to that issue (it might not).

In any case - one possibility if you don't want to create
a new CompositeType might be to pass a filtering lambda
to isTypeMatched.

Something like a BiPredicate that
would take a composite type and a field name and tell
whether to include it or not in the comparison.

best regards,

-- daniel


On 16/02/2018 05:01, Jeremy Manson wrote:



On Thu, Feb 15, 2018 at 6:40 PM, David Holmes > wrote:


On 16/02/2018 11:12 AM, Jeremy Manson wrote:

Previous bug:

https://bugs.openjdk.java.net/browse/JDK-6588467


And the review thread:


http://mail.openjdk.java.net/pipermail/serviceability-dev/2015-January/016356.html



I don't think the bug would have been obvious to a reviewer (or,
indeed, the author of the patch!), because we would have had to
think about how ticd.isCurrentVersion worked, and noticed the
fact that some of the fields are optional.


I misunderstood the connection to the old bug and review. This is a
pre-existing issue that wasn't noticed last time this code was
updated - right?


Sort of.  The uses of isCurrentVersion() didn't happen to need to 
exclude optional values before I made my changes.  It falls under the 
category of unstated assumptions.  The method could have been called 
"isCurrentVersionAndDoesntIgnoreOptionalValues".  :)


Jeremy