Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8275259: Add support for Java level DCmd #5938

Closed
wants to merge 2 commits into from
Closed

Conversation

D-D-H
Copy link
Contributor

@D-D-H D-D-H commented Oct 14, 2021

(RFC: https://mail.openjdk.java.net/pipermail/serviceability-dev/2021-October/039796.html)

I proposed to extend DCmd to allow Java developers to customize their own diagnostic commands last week.

At present, I have implemented a preliminary version.

In the current implementation, I provided some simple APIs in the Java layer (under sun.management.cmd) for defining and registering commands.

  • Executable interface
    Java diagnostic commands need to implement this interface, the interface only contains a simple method:
    /**
     * @param output the output when this executable is running
     */
    void execute(PrintWriter output);
  • Add two annotations (@command and @parameter) to describe the command meta info

  • Use Factory API to register command, the following forms are supported

@Command(name = "My.Echo", description = "Echo description")
class Echo implements Executable {

    @Parameter(name = "text", ordinal=0, isMandatory = true)
    String text;

    @Parameter(name = "repeat", isMandatory = true, defaultValue = "1")
    int repeat;

    @Override
    public void execute(PrintWriter out) {
        for (int i = 0 ; i < repeat; i++) {
            out.println(text);
        }
    }
}

Factory.register(Echo.class);
Factory.register("My.Date", output -> {
    output.println(new Date());
});
  • When the command is running, the VM will call Executor.executeCommand to execute the command. In the implementation of Executor, I introduced a simple timeout mechanism to prevent the command channel from being blocked.

At the VM layer, I extended the existing DCmd framework(implemented JavaDCmd and JavaDCmdFactoryImpl) to be compatible with existing functions (jmx, help, etc.).

In terms of security, considering that the SecurityManager will be deprecated, there are not too many considerations for the time being.

Any input is appreciated.

Thanks,
Denghui


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5938/head:pull/5938
$ git checkout pull/5938

Update a local copy of the PR:
$ git checkout pull/5938
$ git pull https://git.openjdk.java.net/jdk pull/5938/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5938

View PR using the GUI difftool:
$ git pr show -t 5938

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5938.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 14, 2021

👋 Welcome back ddong! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Oct 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 14, 2021

@D-D-H The following labels will be automatically applied to this pull request:

  • hotspot
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added serviceability hotspot labels Oct 14, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 14, 2021

Webrevs

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Oct 14, 2021

/label add hotspot-runtime

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Oct 14, 2021

/label remove hotspot

@openjdk openjdk bot added the hotspot-runtime label Oct 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 14, 2021

@D-D-H
The hotspot-runtime label was successfully added.

@openjdk openjdk bot removed the hotspot label Oct 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 14, 2021

@D-D-H
The hotspot label was successfully removed.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Oct 14, 2021

In the current implementation, I provided some simple APIs in the Java layer (under sun.management.cmd)

An API that is expected to be used by application code can't go in the unsupported/internal-use-only sun.management namespace. This would have to be a new external facing supported API.

Cheers,
David

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Oct 14, 2021

In the current implementation, I provided some simple APIs in the Java layer (under sun.management.cmd)

An API that is expected to be used by application code can't go in the unsupported/internal-use-only sun.management namespace. This would have to be a new external facing supported API.

Cheers, David

Thanks.
I consider putting the API into javax.management.dcmd. What do you think?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 14, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 14/10/2021 4:32 pm, Denghui Dong wrote:

On Thu, 14 Oct 2021 06:17:13 GMT, David Holmes <dholmes at openjdk.org> wrote:

In the current implementation, I provided some simple APIs in the Java layer (under sun.management.cmd)

An API that is expected to be used by application code can't go in the unsupported/internal-use-only sun.management namespace. This would have to be a new external facing supported API.

Cheers, David

Thanks.
I consider putting the API into `javax.management.dcmd`. What do you think?

There are very specific rules related to the use of the javax namespace.

I'm not sure exactly where this API would need to go. IIUC jcmd doesn't
exist at the Java level it is just a tool, so introducing an API to
interact with it seems problematic to me. Other folk in serviceability
(both VM and JDK sides) will need to weigh in here.

David

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Oct 14, 2021

I'm not sure exactly where this API would need to go. IIUC jcmd doesn't
exist at the Java level it is just a tool, so introducing an API to
interact with it seems problematic to me.

IMO, jcmd is a client of dcmd, the user also can use JMX to execute dcmd, this means there already are Java APIs interact with dcmd.

Denghui

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 14, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 14/10/2021 5:14 pm, Denghui Dong wrote:

On Thu, 14 Oct 2021 06:43:20 GMT, David Holmes <david.holmes at oracle.com> wrote:

I'm not sure exactly where this API would need to go. IIUC jcmd doesn't
exist at the Java level it is just a tool, so introducing an API to
interact with it seems problematic to me.

IMO, `jcmd` is a client of `dcmd`, the user also can use JMX to execute `dcmd`, this means there already are Java APIs interact with `dcmd`.

Ah right - so you're really looking at extending the capabilities of the
DiagnosticCommandMBean to add a way to register a Java diagnostic command.

David

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Oct 14, 2021

Ah right - so you're really looking at extending the capabilities of the
DiagnosticCommandMBean to add a way to register a Java diagnostic command.

Yes, but we could not add registration API to DiagnosticCommandMBean directly.

There are already some VM diagnostic commands that depend on Java methods directly, this extension could also bring benefit to this scenario I think.

Hope folks from serviceability could take a look at it:)

Denghui

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Oct 14, 2021

Hi,

Interesting proposal. I have some questions.

  • Will hanging java user code block the attach listener thread? If yes, how would you solve that?
  • Will this require changes to the jcmd client, or will this work with any jcmd client, up- and downward the JDK versions? (the nice thing about jcmd is that all the logic resides at the hotspot and I can use any client to talk to any hotspot)

Cheers, Thomas

P.S. I think it may be worthwhile to discuss this on the serviceability-dev mailing list first.

@egahlin
Copy link
Member

@egahlin egahlin commented Oct 14, 2021

To understand what is needed (namespacing, error handling, safety, lifecycle etc.) and if this is actually useful for Java, I think you need to write 2-3 commands for a few popular frameworks, for example Spring.

If we can't come up with 5-10 commands where this functionality would solve real problems users face, it's unlikely somebody will use it. It will just be a maintenance burden in the JDK. Once an API is added, it's hard to remove.

The scope of this seem to be JEP level.

(JFR is not a valid use case, since we don't want to add a dependency on annotation classes in the jdk.jcmd module).

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Oct 15, 2021

Hi Thomas,

Hi,

Interesting proposal. I have some questions.

  • Will hanging java user code block the attach listener thread? If yes, how would you solve that?

In my current implementation, I use a simple timeout mechanism to solve this problem.

Create a Thread to run the command, and use java.util.concurrent.Future#get(long, java.util.concurrent.TimeUnit) to get the result.

For more details please refer to Executor.java.

  • Will this require changes to the jcmd client, or will this work with any jcmd client, up- and downward the JDK versions? (the nice thing about jcmd is that all the logic resides at the hotspot and I can use any client to talk to any hotspot)

At present, I only extend the DCMD framework and do not modify the code of jcmd. So it works with any jcmd client and jmx client, up and downward JDK versions I think.

Cheers, Thomas

P.S. I think it may be worthwhile to discuss this on the serviceability-dev mailing list first.

Denghui

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Oct 15, 2021

Hi Erik,

To understand what is needed (namespacing, error handling, safety, lifecycle etc.) and if this is actually useful for Java, I think you need to write 2-3 commands for a few popular frameworks, for example Spring.

If we can't come up with 5-10 commands where this functionality would solve real problems users face, it's unlikely somebody will use it. It will just be a maintenance burden in the JDK. Once an API is added, it's hard to remove.

The scope of this seem to be JEP level.

(JFR is not a valid use case, since we don't want to add a dependency on annotation classes in the jdk.jcmd module).

Thanks for the comments.

I will investigate whether there is a scenario to apply this extension in the current mainstream framework.

Denghui

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Oct 15, 2021

Hi Thomas,

Hi,
Interesting proposal. I have some questions.

  • Will hanging java user code block the attach listener thread? If yes, how would you solve that?

In my current implementation, I use a simple timeout mechanism to solve this problem.

Create a Thread to run the command, and use java.util.concurrent.Future#get(long, java.util.concurrent.TimeUnit) to get the result.

For more details please refer to Executor.java.

  • Will this require changes to the jcmd client, or will this work with any jcmd client, up- and downward the JDK versions? (the nice thing about jcmd is that all the logic resides at the hotspot and I can use any client to talk to any hotspot)

At present, I only extend the DCMD framework and do not modify the code of jcmd. So it works with any jcmd client and jmx client, up and downward JDK versions I think.

Cheers, Thomas
P.S. I think it may be worthwhile to discuss this on the serviceability-dev mailing list first.

Denghui

Hi Denghui,

thanks for your answers. I'm ambivalent here. I worry about the growing complexity of jcmd, and about unforeseen consequences if we open this door. OTOH I can see this as a useful feature. I'll wait for a consensus.

Thanks, Thomas

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Oct 25, 2021

Seems no more comments on this issue, but I still think this is a useful feature and want it could be a standard feature.

The following is my thought.

The idea of ​​this feature originally came from our internal Java agent that detects the misusage of Unsafe API.

This agent can collect the call sites that allocate or free direct memory in the application(NMT could not do it IMO) to detect direct memory leaks.

In the beginning, it just prints call sites, without any statistical function, it's hard to use.

So we plan to use a way similar to jeprof (from jemalloc) to generate a report file that aggregates all useful information.

During the implementation process, we found that we need a mechanism to notify the agent to generate reports.
(Multiple unrelated dependent components in a Java application may require such a mechanism)

The common practice is:
a) Register a service port, triggered by an HTTP request
b) Triggered by signal
c) Generate reports periodically, or when the process exits

But these three ways have certain problems.
For a) we need to introduce a network component, will increase the complexity of implementation
For b) we cannot pass parameters
For c) some files that may never be used will be generated

Essentially, this question is how to notify the application to do a certain task, or in other words, how do we issue a command to the application.

Naturally, we think that jcmd can already issue some commands registered in VM to the application, why can't we extend to the java layer?

This feature will be very useful for some lightweight tools, just like the scenario we encountered, to notify the tools to perform certain operations.

In addition, this feature will also bring benefits to Java beginners.
For example, at the beginning, beginners may not use advanced log components, but they will also encounter the need to output debug logs. They may write code like this:

if (debug) {
  System.out.println("...");
}

If developers can easily and dynamically control the value of debug, it's attractive.

Factory.register("MyApp.flipDebug", out -> debug = !debug);

jcmd <pid> MyApp.flipDebug

I agree with what Erik said, we need to find applicable scenarios in some mainstream frameworks. After my preliminary investigation, I think we can apply this feature in the features of health checks, graceful shutdown, and dynamic configuration updates.

But to be honest, these frameworks are very mature and stable, and for compatibility purposes, it's hard to let them use this extension.

In terms of code maintainability, the implementation of this extension does not need to destroy the existing framework. The current implementation only extends the DCmd framework and provides several simple APIs for users to use. I think there will not be too many costs after it's stable.

If this feature could become a standard feature, I will be the first user:)

Best,
Denghui

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Oct 25, 2021

Hi Denghui,

as I wrote before, I am neutral on this. But I agree with @egahlin that this seems to be more on JEP level. A PR is not the right first step for this, and I am afraid you won't build much consensus here. Maybe discuss this first on the serviceability ML? Or on hotspot-dev?

Kind Regards, Thomas

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Oct 25, 2021

Hi Denghui,

as I wrote before, I am neutral on this. But I agree with @egahlin that this seems to be more on JEP level. A PR is not the right first step for this, and I am afraid you won't build much consensus here. Maybe discuss this first on the serviceability ML? Or on hotspot-dev?

Kind Regards, Thomas

Okay, thanks!
Will send an RFC discussion mail (RFC: https://mail.openjdk.java.net/pipermail/serviceability-dev/2021-October/039796.html).

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 9, 2021

Mailing list message from Chris Plummer on hotspot-runtime-dev:

On 10/14/21 12:38 AM, David Holmes wrote:

On 14/10/2021 5:14 pm, Denghui Dong wrote:

On Thu, 14 Oct 2021 06:43:20 GMT, David Holmes
<david.holmes at oracle.com> wrote:

I'm not sure exactly where this API would need to go. IIUC jcmd doesn't
exist at the Java level it is just a tool, so introducing an API to
interact with it seems problematic to me.

IMO, `jcmd` is a client of `dcmd`, the user also can use JMX to
execute `dcmd`, this means there already are Java APIs interact with
`dcmd`.

Ah right - so you're really looking at extending the capabilities of
the DiagnosticCommandMBean to add a way to register a Java diagnostic
command.

David

Just reviewing some previous emails and noticed this one since I
recently looked into the JMX support. JMX supports a rather generic
concept of executing diagnostics commands (there is no actual reference
to "dcmds"). See com.sun.management.DiagnosticCommandMBean and its
superinterface javax.management.DynamicMBean:

https://docs.oracle.com/en/java/javase/15/docs/api/jdk.management/com/sun/management/DiagnosticCommandMBean.html
https://docs.oracle.com/en/java/javase/15/docs/api/java.management/javax/management/DynamicMBean.html

DynamicMBean provides the invoke() interface, which when used on a
DiagnosticCommandMBean will invoke the specified diagnostic command. It
says nothing about how diagnostic commands are implemented or registered
with the jvm. That I assume would be JVM implementation dependent.

Ah right - so you're really looking at extending the capabilities of
the DiagnosticCommandMBean to add a way to register a Java diagnostic
command.

Yes to "add a way to register a Java diagnostic command", but the new
API will not involve DiagnosticCommandMBean in any way. The new API will
interface with the existing hotspot native support for dcmds to allow
dcmds to be implemented by and registered from java.

In any case, I think having an API that allows you to invoke a
diagnostic command from java is very different in scope from an API that
allows you to implement one in java, so I'm not so sure this JMX
precedence serves us much purpose here.

Chris

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 9, 2021

Mailing list message from Ioi Lam on hotspot-runtime-dev:

On 11/9/21 2:07 PM, Chris Plummer wrote:

On 10/14/21 12:38 AM, David Holmes wrote:

On 14/10/2021 5:14 pm, Denghui Dong wrote:

On Thu, 14 Oct 2021 06:43:20 GMT, David Holmes
<david.holmes at oracle.com> wrote:

I'm not sure exactly where this API would need to go. IIUC jcmd
doesn't
exist at the Java level it is just a tool, so introducing an API to
interact with it seems problematic to me.

IMO, `jcmd` is a client of `dcmd`, the user also can use JMX to
execute `dcmd`, this means there already are Java APIs interact with
`dcmd`.

Ah right - so you're really looking at extending the capabilities of
the DiagnosticCommandMBean to add a way to register a Java diagnostic
command.

David

Just reviewing some previous emails and noticed this one since I
recently looked into the JMX support. JMX supports a rather generic
concept of executing diagnostics commands (there is no actual
reference to "dcmds"). See com.sun.management.DiagnosticCommandMBean
and its superinterface javax.management.DynamicMBean:

https://docs.oracle.com/en/java/javase/15/docs/api/jdk.management/com/sun/management/DiagnosticCommandMBean.html

https://docs.oracle.com/en/java/javase/15/docs/api/java.management/javax/management/DynamicMBean.html

DynamicMBean provides the invoke() interface, which when used on a
DiagnosticCommandMBean will invoke the specified diagnostic command.
It says nothing about how diagnostic commands are implemented or
registered with the jvm. That I assume would be JVM implementation
dependent.

Ah right - so you're really looking at extending the capabilities of
the DiagnosticCommandMBean to add a way to register a Java diagnostic
command.
Yes to "add a way to register a Java diagnostic command", but the new
API will not involve DiagnosticCommandMBean in any way. The new API
will interface with the existing hotspot native support for dcmds to
allow dcmds to be implemented by and registered from java.

In any case, I think having an API that allows you to invoke a
diagnostic command from java is very different in scope from an API
that allows you to implement one in java, so I'm not so sure this JMX
precedence serves us much purpose here.

Is there anything in the proposal that cannot be done by JMX? If the app
wants to provide a hook to expose some of its internals to be
monitored/controlled by an external tool, maybe it can just implement a
new MBean?

Granted, it's bit more tedious to connect to JMX than using "jcmd".
However, much of that is for security. If we allow an app to implement
new jcmd handlers using Java code, we have to think about the whole
security model again. Are we reinventing the wheel here?

Thanks
- Ioi

@D-D-H
Copy link
Contributor Author

@D-D-H D-D-H commented Nov 10, 2021

Thanks for the commands, the following is my thought.

Compared with the user's own implementation of a DiagnosticCommandMBean, the main benefits of this extension are:

Users can directly use jcmd to invoke (better user experience, of course, users can also invoke through a jmx client like jconsole), and use jcmd pid help to list all VM commands and user-defined commands. And all parameters or options are pre-parsed, making the implementation of the command simpler and focusing on their own logic.

I am not an expert in the security field, but I feel that implementing a DiagnosticCommandMBean by the users does not seem to be more secure than the method in the proposal. I mean the users can still do whatever they want.

Just like Chris said, it seems that not many people are interested in this proposal. I plan to close this PR first.
If there are any others who want this in place, we can continue to discuss it in the RFC thread of this proposal.

Denghui,
Thanks

@D-D-H D-D-H closed this Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime rfr serviceability
4 participants