[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: Review Request JDK-8200559: Java agents doing instrumentation need a means to define auxiliary c
From: Rafael Winterhalter <rafael.wth () gmail ! com>
Date: 2018-05-30 6:58:58
Message-ID: CA+DM0AmvVYyQOPMH_beoPmL3famMhr+9TsP0aLaFCFmfn5D_GA () mail ! gmail ! com
[Download RAW message or body]
Hei Mandy,
yes, this sums it up, I think and I have worked with all use cases where
sun.misc.Unsafe::defineClass is more then sufficient. One major issue I
also have with the current API are the costs in refactoring existing
agents, especially since one would need to deliver a seperate agent for
Java 11+ to rely on the additional API that is not available in previous
versions. Since Java 8 is still popular and most likely will be in many
year to come, I believe this additional maintainance cost would drive the
decision to not use the proposed API.
If one had a direct equivalent - such as Instrumentation::defineClass -
this would just be the easiest path forward and propably result in a quick
migration since current agents can simply call the new method using
reflection where an agent can decide at runtime if Unsafe::defineClass or
Instrumentation::defineClass could be used.
Finally, after I prototyped a bit, I found the proposed solution a bit
difficult to use. For example, if I instrument three classes in the same
package that all communicate via a new class that I inject, I only need to
inject the class in question a single time. However, I cannot always
determine which of the three classes is loaded first as this depends on the
application. Now I need to add some form of atomic boolean to apply the
injection on the first instrumentation but this can also be problematic
since another class might be loaded concurrently by another thread where I
ended up inserting locks etc what yielded even more problems. In the
current code, I just inject the class in question before registering the
transformer to avoid the problem at all.
Therefore I believe that the Instrumentation::defineClass would offer a
best solution given today's situation. The proposed API's capabilities
could easily be emulated by an agent author if this package-local scope was
required for a plugin. Also, since the instrumentation API already offers a
way of defining classes in random packages without using internal API (the
suggested noop redefinition of a class in the target package or by opening
modules with a subsequent use of method handles loopup), this does not
expose new functionality that is not already offered in a less convenient
way. Beyond the workarounds that I already brought up, I know of several
more that are in use today and I believe that by offering an easy way of
class definition as with Instrumentation::defineClass, there is a good
chance that all of those will go away eventually.
Thank you for considering!
Best regards, Rafael
2018-05-30 6:46 GMT+02:00 mandy chung <mandy.chung@oracle.com>:
> Hi Rafael,
>
> Thanks for the additional use cases of Unsafe::defineClass and looking
> through many different agents to identify their migration path.
>
> Summarize the requirements you collected (please correct/add):
>
> 1. need a means to define auxiliary classes in the same runtime package
> of an instrumented class, e.g. to bridge non-public members
>
> 2. need a means to define agent classes in a module that provides
> similar functionality as appendToBootstrapClassLoader and
> appendToSystemClassLoaderSearch​
>
> 3. define a class in a different runtime package as the instrumented
> class. It's still unclear how common this is (inject a subclass
> that depends on a package-private constructor)
>
> The proposed API requires the agent to redefine the class in a
> package that the agent intends to inject its class into (e.g.
> redefine java.util.List in order to define java.util.AgentDispatcher).
> The proposed API requires more work than the existing
> Unsafe::defineClass that can define a class in any package but
> still plausible, right?
>
> We should explore other alternatives, if any.
>
> Mandy
> [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/20
> 18-April/023535.html
>
> On 5/29/18 2:17 PM, Rafael Winterhalter wrote:
>
>> Hei Mandy,
>>
>> after further investigation I have found several other use cases of
>> sun.misc.Unsafe::defineClass for which the proposal for JDK-8200559 would
>> not currently offer a solution. I will try to describe a generic case of
>> the problem I found:
>>
>> Java agents sometimes need to communicate state from instrumented classes
>> to a dispatcher that comes with the agent. However, a Java agent cannot
>> generally expect that any agent classes are visible to an application as a
>> Java agent is loaded on the class path whereas application classes might
>> not be loaded by the system class loader or any of its children. Therefore,
>> instrumented classes cannot typically callback into the agent. This is
>> typically solved by adding classes to the bootstrap class loader which is
>> universally visible.
>>
>> Such a callback typically looks like this:
>>
>> public abstract class Dispatcher {
>> public volatile Dispatcher dispatcher;
>> public abstract void receiveMessage(Object msg);
>> public static void sendMessage(Object msg) {
>> Dispatcher d = dispatcher;
>> if (d != null) d.receiveMessage(msg);
>> }
>> }
>>
>> The agent can now register a subclass of the dispatcher in the field and
>> receive messages from any instrumented code throughout an application
>> without considering class loaders.
>>
>> Adding this call would be trivial by using the
>> Instrumentation::appendToBootSearchPath method. However, using this
>> method has two major downsides:
>> 1. The method accepts a file which must be stored on the local file
>> system. This can be really tricky in some cases, it is typically easier to
>> just define the classes as byte arrays and sun.misc.Unsafe::defineClass is
>> a way to avoid this problem.
>> 2. Appending to the search path is only possible in the unnamed module
>> since Java 9. It is no longer possible to define the above dispatcher to be
>> located in java.util for instance as the new standard class loader does not
>> consider the new jar for known modules anymore. Defining the dispatcher in
>> a well known package has two major advantages:
>> a) It is not necessary to alter the module graph to make sure that all
>> modules with instrumented classes read the boot loaders unnamed module.
>> This requires a lot of additional maintenance which impacts performance and
>> memory use. Also, bugs can trigger security risks if edges are added
>> excessively. With defining a class in java.util, this can be avoided since
>> the java.base module is impliiclty read by any module.
>> b) Some class loaders filter packages that are considered to be
>> loaded, especially OSGi and JBoss modules class loaders. Those class
>> loaders would for example never attempt to find classes in the com.acme
>> package on the bootstrap loader which is why it has become a popular
>> practice to add classes to java.* packages for such universal dispatch.
>> There are dozens of such restricting class loaders and working around them
>> is nearly impossible as there are so many.
>>
>> With the current consideration, it would instead be required to "pseudo
>> redefine" a class such as java.util.List only to inject the dispatcher into
>> the desired package. (Alternatively one can also open the module of
>> java.base and use a method handles lookup but this is a bad idea if the
>> Java agent runs on the unnamed system class loader.) Note that all this is
>> possible using standard API. It is even easier to do with JVMTI where class
>> definition is trivial.
>>
>> For all the reasons I have mentioned in previous mails and also for this
>> additional use case I hope that adding a method for defining a class can be
>> added to the Instrumentation interface, it would definitely allow for the
>> cleanest, fastest and least error-prone migration while not inviting to the
>> mentioned work-arounds which are also currently favored by most teams
>> working with Java agents that I know of where many of them just open
>> jdk.internal.misc to use the new Unsafe class what is really unfortunate as
>> this is a chance to avoid use of internal API alltogether. This choice
>> would also reduce additional time for Java 11 being supported by all major
>> production monitoring tools that could just add simple switches to their
>> code to move from Unsafe to Instrumentation.
>>
>> It would also be trivial to implement with an interface extension like:
>>
>> interface Instrumentation {
>> default Class<?> defineClass(byte[] bytes, ClassLoader loader,
>> ProtectionDomain pd) { throw new UnsupportedOperationException("defineClass");
>> }
>> }
>>
>> Thanks for considering my suggestion and your feedback!
>> Best regards, Rafael
>>
>>
>>
>> 2018-04-15 8:23 GMT+02:00 mandy chung <mandy.chung@oracle.com <mailto:
>> mandy.chung@oracle.com>>:
>>
>>
>> Background:
>>
>> Java agents support both load time and dynamic instrumentation. At
>> load time,
>> the agent's ClassFileTransformer is invoked to transform class
>> bytes. There is
>> no Class objects at this time. Dynamic instrumentation is when
>> redefineClasses
>> or retransformClasses is used to redefine an existing loaded class.
>> The
>> ClassFileTransformer is invoked with class bytes where the Class
>> object is present.
>>
>> Java agent doing instrumentation needs a means to define auxiliary
>> classes
>> that are visible and accessible to the instrumented class. Existing
>> agents
>> have been using sun.misc.Unsafe::defineClass to define aux classes
>> directly
>> or accessing protected ClassLoader::defineClass method with
>> setAccessible to
>> suppress the language access check (see [1] where this issue was
>> brought up).
>>
>> Instrumentation::appendToBootstrapClassLoaderSearch and
>> appendToSystemClassLoaderSearch
>> APIs are existing means to supply additional classes. It's too
>> limited
>> for example it can't inject a class in the same runtime package as
>> the class
>> being transformed.
>>
>> Proposal:
>>
>> This proposes to add a new ClassFileTransformer.transform method
>> taking additional ClassDefiner parameter. A transformer can define
>> additional
>> classes during the transformation process, i.e.
>> when ClassFileTransformer::transform is invoked. Some details:
>>
>> 1. ClassDefiner::defineClass defines a class in the same runtime
>> package
>> as the class being transformed.
>> 2. The class is defined in the same thread as the transformers are
>> being
>> invoked. ClassDefiner::defineClass returns Class object directly
>> before the transformed class is defined.
>> 3. No transformation is applied to classes defined by
>> ClassDefiner::defineClass.
>>
>> The first prototype we did is to collect the auxiliary classes and
>> define
>> them until all transformers are invoked and have these aux classes
>> to go
>> through the transformation pipeline. Several complicated issues would
>> need to be resolved for example timing whether the auxiliary classes
>> should
>> be defined before the transformed class (otherwise a potential race
>> where
>> some other thread references the transformed class and cause the code
>> to
>> execute that in turn reference the auxiliary classes. The current
>> implementation has a native reentrancy check that ensure one class
>> is being
>> transformed to avoid potential circularity issues. This may need
>> JVM TI
>> support to be reliable.
>>
>> This proposal would allow java agents to migrate from internal API
>> and ClassDefiner to be enhanced in the future.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8200559/webrev.00/
>> <http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8200559/webrev.00/>
>>
>> Mandy
>> [1]
>> http://mail.openjdk.java.net/pipermail/jdk-dev/2018-January/
>> 000405.html
>> <http://mail.openjdk.java.net/pipermail/jdk-dev/2018-January
>> /000405.html>
>>
>>
>>
[Attachment #3 (text/html)]
<div dir="ltr"><div>Hei Mandy,</div><div><br></div><div>yes, this sums it up, I think \
and I have worked with all use cases where sun.misc.Unsafe::defineClass is more then \
sufficient. One major issue I also have with the current API are the costs in \
refactoring existing agents, especially since one would need to deliver a seperate \
agent for Java 11+ to rely on the additional API that is not available in previous \
versions. Since Java 8 is still popular and most likely will be in many year to come, \
I believe this additional maintainance cost would drive the decision to not use the \
proposed API.</div><div><br></div><div>If one had a direct equivalent - such as \
Instrumentation::defineClass - this would just be the easiest path forward and \
propably result in a quick migration since current agents can simply call the new \
method using reflection where an agent can decide at runtime if Unsafe::defineClass \
or Instrumentation::defineClass could be used.</div><div><br></div><div>Finally, \
after I prototyped a bit, I found the proposed solution a bit difficult to use. For \
example, if I instrument three classes in the same package that all communicate via a \
new class that I inject, I only need to inject the class in question a single time. \
However, I cannot always determine which of the three classes is loaded first as this \
depends on the application. Now I need to add some form of atomic boolean to apply \
the injection on the first instrumentation but this can also be problematic since \
another class might be loaded concurrently by another thread where I ended up \
inserting locks etc what yielded even more problems. In the current code, I just \
inject the class in question before registering the transformer to avoid the problem \
at all.</div><div><br></div><div>Therefore I believe that the \
Instrumentation::defineClass would offer a best solution given today's situation. \
The proposed API's capabilities could easily be emulated by an agent author if \
this package-local scope was required for a plugin. Also, since the instrumentation \
API already offers a way of defining classes in random packages without using \
internal API (the suggested noop redefinition of a class in the target package or by \
opening modules with a subsequent use of method handles loopup), this does not expose \
new functionality that is not already offered in a less convenient way. Beyond the \
workarounds that I already brought up, I know of several more that are in use today \
and I believe that by offering an easy way of class definition as with \
Instrumentation::defineClass, there is a good chance that all of those will go away \
eventually.</div><div><br></div><div>Thank you for considering! <br></div><div>Best \
regards, Rafael<br></div></div><div class="gmail_extra"><br><div \
class="gmail_quote">2018-05-30 6:46 GMT+02:00 mandy chung <span dir="ltr"><<a \
href="mailto:mandy.chung@oracle.com" \
target="_blank">mandy.chung@oracle.com</a>></span>:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">Hi Rafael,<br> <br>
Thanks for the additional use cases of Unsafe::defineClass and looking<br>
through many different agents to identify their migration path.<br>
<br>
Summarize the requirements you collected (please correct/add):<br>
<br>
1. need a means to define auxiliary classes in the same runtime package<br>
of an instrumented class, e.g. to bridge non-public members<br>
<br>
2. need a means to define agent classes in a module that provides<br>
similar functionality as appendToBootstrapClassLoader and<br>
<wbr>appendToSystemClassLoaderSearc<wbr>h​<br>
<br>
3. define a class in a different runtime package as the instrumented<br>
class. It's still unclear how common this is (inject a subclass<br>
that depends on a package-private constructor)<br>
<br>
The proposed API requires the agent to redefine the class in a<br>
package that the agent intends to inject its class into (e.g.<br>
redefine java.util.List in order to define java.util.AgentDispatcher).<br>
The proposed API requires more work than the existing<br>
Unsafe::defineClass that can define a class in any package but<br>
still plausible, right?<br>
<br>
We should explore other alternatives, if any.<br>
<br>
Mandy<br>
[1] <a href="http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-April/023535.html" \
rel="noreferrer" target="_blank">http://mail.openjdk.java.net/p<wbr>ipermail/serviceability-dev/20<wbr>18-April/023535.html</a><span \
class=""><br> <br>
On 5/29/18 2:17 PM, Rafael Winterhalter wrote:<br>
</span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><span class=""> Hei Mandy,<br>
<br>
after further investigation I have found several other use cases of \
sun.misc.Unsafe::defineClass for which the proposal for JDK-8200559 would not \
currently offer a solution. I will try to describe a generic case of the problem I \
found:<br> <br>
Java agents sometimes need to communicate state from instrumented classes to a \
dispatcher that comes with the agent. However, a Java agent cannot generally expect \
that any agent classes are visible to an application as a Java agent is loaded on the \
class path whereas application classes might not be loaded by the system class loader \
or any of its children. Therefore, instrumented classes cannot typically callback \
into the agent. This is typically solved by adding classes to the bootstrap class \
loader which is universally visible.<br> <br>
Such a callback typically looks like this:<br>
<br>
public abstract class Dispatcher {<br>
public volatile Dispatcher dispatcher;<br>
public abstract void receiveMessage(Object msg);<br>
public static void sendMessage(Object msg) {<br>
Dispatcher d = dispatcher;<br>
if (d != null) d.receiveMessage(msg);<br>
}<br>
}<br>
<br>
The agent can now register a subclass of the dispatcher in the field and receive \
messages from any instrumented code throughout an application without considering \
class loaders.<br> <br>
Adding this call would be trivial by using the \
Instrumentation::appendToBootS<wbr>earchPath method. However, using this method has \
two major downsides:<br> 1. The method accepts a file which must be stored on the \
local file system. This can be really tricky in some cases, it is typically easier to \
just define the classes as byte arrays and sun.misc.Unsafe::defineClass is a way to \
avoid this problem.<br> 2. Appending to the search path is only possible in the \
unnamed module since Java 9. It is no longer possible to define the above dispatcher \
to be located in java.util for instance as the new standard class loader does not \
consider the new jar for known modules anymore. Defining the dispatcher in a well \
known package has two major advantages:<br> a) It is not necessary to alter the \
module graph to make sure that all modules with instrumented classes read the boot \
loaders unnamed module. This requires a lot of additional maintenance which impacts \
performance and memory use. Also, bugs can trigger security risks if edges are added \
excessively. With defining a class in java.util, this can be avoided since the \
java.base module is impliiclty read by any module.<br> b) Some class loaders filter \
packages that are considered to be loaded, especially OSGi and JBoss modules class \
loaders. Those class loaders would for example never attempt to find classes in the \
com.acme package on the bootstrap loader which is why it has become a popular \
practice to add classes to java.* packages for such universal dispatch. There are \
dozens of such restricting class loaders and working around them is nearly impossible \
as there are so many.<br> <br>
With the current consideration, it would instead be required to "pseudo \
redefine" a class such as java.util.List only to inject the dispatcher into the \
desired package. (Alternatively one can also open the module of java.base and use a \
method handles lookup but this is a bad idea if the Java agent runs on the unnamed \
system class loader.) Note that all this is possible using standard API. It is even \
easier to do with JVMTI where class definition is trivial.<br> <br>
For all the reasons I have mentioned in previous mails and also for this additional \
use case I hope that adding a method for defining a class can be added to the \
Instrumentation interface, it would definitely allow for the cleanest, fastest and \
least error-prone migration while not inviting to the mentioned work-arounds which \
are also currently favored by most teams working with Java agents that I know of \
where many of them just open jdk.internal.misc to use the new Unsafe class what is \
really unfortunate as this is a chance to avoid use of internal API alltogether. This \
choice would also reduce additional time for Java 11 being supported by all major \
production monitoring tools that could just add simple switches to their code to move \
from Unsafe to Instrumentation.<br> <br>
It would also be trivial to implement with an interface extension like:<br>
<br>
interface Instrumentation {<br>
default Class<?> defineClass(byte[] bytes, ClassLoader loader, \
ProtectionDomain pd) { throw new \
UnsupportedOperationException(<wbr>"defineClass"); }<br> }<br>
<br>
Thanks for considering my suggestion and your feedback!<br>
Best regards, Rafael<br>
<br>
<br>
<br></span>
2018-04-15 8:23 GMT+02:00 mandy chung <<a href="mailto:mandy.chung@oracle.com" \
target="_blank">mandy.chung@oracle.com</a> <mailto:<a \
href="mailto:mandy.chung@oracle.com" \
target="_blank">mandy.chung@oracle.com</a><wbr>>>:<div><div class="h5"><br> \
<br> Background:<br>
<br>
Java agents support both load time and dynamic instrumentation. At<br>
load time,<br>
the agent's ClassFileTransformer is invoked to transform class<br>
bytes. There is<br>
no Class objects at this time. Dynamic instrumentation is when<br>
redefineClasses<br>
or retransformClasses is used to redefine an existing loaded class. The<br>
ClassFileTransformer is invoked with class bytes where the Class<br>
object is present.<br>
<br>
Java agent doing instrumentation needs a means to define auxiliary<br>
classes<br>
that are visible and accessible to the instrumented class. Existing<br>
agents<br>
have been using sun.misc.Unsafe::defineClass to define aux classes<br>
directly<br>
or accessing protected ClassLoader::defineClass method with<br>
setAccessible to<br>
suppress the language access check (see [1] where this issue was<br>
brought up).<br>
<br>
Instrumentation::appendToBoots<wbr>trapClassLoaderSearch and<br>
appendToSystemClassLoaderSearc<wbr>h<br>
APIs are existing means to supply additional classes. It's too \
limited<br>
for example it can't inject a class in the same runtime package as<br>
the class<br>
being transformed.<br>
<br>
Proposal:<br>
<br>
This proposes to add a new ClassFileTransformer.transform method<br>
taking additional ClassDefiner parameter. A transformer can define<br>
additional<br>
classes during the transformation process, i.e.<br>
when ClassFileTransformer::transfor<wbr>m is invoked. Some details:<br>
<br>
1. ClassDefiner::defineClass defines a class in the same runtime package<br>
as the class being transformed.<br>
2. The class is defined in the same thread as the transformers are being<br>
invoked. ClassDefiner::defineClass returns Class object directly<br>
before the transformed class is defined.<br>
3. No transformation is applied to classes defined by<br>
ClassDefiner::defineClass.<br>
<br>
The first prototype we did is to collect the auxiliary classes and<br>
define<br>
them until all transformers are invoked and have these aux classes<br>
to go<br>
through the transformation pipeline. Several complicated issues would<br>
need to be resolved for example timing whether the auxiliary classes<br>
should<br>
be defined before the transformed class (otherwise a potential race<br>
where<br>
some other thread references the transformed class and cause the code to<br>
execute that in turn reference the auxiliary classes. The current<br>
implementation has a native reentrancy check that ensure one class<br>
is being<br>
transformed to avoid potential circularity issues. This may need<br>
JVM TI<br>
support to be reliable.<br>
<br>
This proposal would allow java agents to migrate from internal API<br>
and ClassDefiner to be enhanced in the future.<br>
<br>
Webrev:<br>
<a href="http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8200559/webrev.00/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~mc<wbr>hung/jdk11/webrevs/8200559/web<wbr>rev.00/</a><br>
<<a href="http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8200559/webrev.00/" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~m<wbr>chung/jdk11/webrevs/8200559/we<wbr>brev.00/</a>><br>
<br>
Mandy<br>
[1]<br>
<a href="http://mail.openjdk.java.net/pipermail/jdk-dev/2018-January/000405.html" \
rel="noreferrer" target="_blank">http://mail.openjdk.java.net/p<wbr>ipermail/jdk-dev/2018-January/<wbr>000405.html</a><br></div></div>
<<a href="http://mail.openjdk.java.net/pipermail/jdk-dev/2018-January/000405.html" \
rel="noreferrer" target="_blank">http://mail.openjdk.java.net/<wbr>pipermail/jdk-dev/2018-January<wbr>/000405.html</a>><br>
<br>
<br>
</blockquote>
</blockquote></div><br></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic