Review Request JDK-8181443: Replace usages of jdk.internal.misc.Unsafe with MethodHandles.Lookup.defineClass

[Rafael Winterhalter]

Hi Alan,

yes, I remember the discussion well. Unfortunately, I did not get the
impression that my argument was heard or even considered. I mentioned the
possible positive effects to mocking in Mockito as a supportive argument
but not at all as my main point. From my experience of implementing a good
dozed of agents for various customers and from consulting for several major
Java tool vendors, I argue that the suggested API does not sufficiently
cover many of the mentioned "benign" instrumentation use cases. I fully
agree that Unsafe needs to be dismantled but we should neither forget how
much good came from the class when restricting its use and that we should
retain these use cases as good as possible. Monitoring agents are used in a
majority of production environments nowadays, it would be a shame if some
of those would suddenly stop working or had to restrict their feature set.

To make sure that this is considered properly, I want to lay out a simple
but common instrumentation use case once again to show how the current API
suggestion is too limited. Consider a basic instrumentation for monitoring
the runtime of a business transaction that spans to classes. I keep it
fairly simple but this is not too far off to how some monitoring tools are
actually implemented. Given the state:

package foo;
public class Foo {
  Bar bar;
  public void foo() {
    // some expensive operation
    bar.bar(new SomeRunnable());
  }
}

package bar;
public class Bar {
  public void bar(Runnable r) {
    r.run();
   // some expensive operation
  }
}

a Java agent is used to inject a class and to instrument the above classes
into:

package foo;
public class Foo {
  Bar bar;
  public void foo() {
    long time = System.currentTimeMillis();
    // some expensive operation
    bar.bar(new InjectedRunnable(new SomeRunnable(), time));
  }
}

package foo;
public class InjectedRunnable implements Runnable {
  private final Runnable delegate;
  public final long time;
  public void run () { delegate.run(); }
}

package bar;
public class Bar {
  public void bar(Runnable r) {
    r.run();
    // some expensive operation
    if (r instanceof InjectedRunnable) {
      System.out.println(System.currentTimeMillis() - ((InjectedRunnable)
r).time);
    }
  }
}

With restricting class injection to a package, the InjectedRunnable cannot
be injected upon loading bar.Bar but only upon loading foo.Foo. This means
that if bar.Bar was loaded and used before foo.Foo, a NoClassDefFoundError
is thrown. As a Java agent cannot controll the order of class loading, the
only alternative is delaying the instrumentation of bar.Bar until foo.Foo
was loaded such that foo.InjectedRunnable is guaranteed to exist. This
possibily requires a retransformation of bar.Bar which is obviously adding
to runtime costs, especially if several such combined instrumentations are
applied by an agent.

Another alternative is to inject the callback into the package of whatever
class is loaded first and condition the instrumentation upon it. But due to
module boundaries, this is not always possible unless one is willing to
break encapsulation of modules based on class loading order what I find a
doubtful approach.

I have taken the proposal very seriously and I have spent a few days of my
own time to battle test it in real-life agents that are widely used in the
Java space. With a lot of effort, you can make it work but at a minimum,
jumping through the additional hoops adds up to several seconds of VM
startup time besides cluttering the agent code base. I do not think that
this is a desirable outcome of an API evolution and I find it a bit
disappointing that my efforts and feedback are just discarded as not being
"benign instrumentation".

That said, the easiest migration path is to implement your own
Unsafe::defineClass as:

class UnsafeDefineClassProxy {
  void foo(Instrumentation inst, Class<?> packageOwner, byte[] bytes) {
    inst.addClassFileTransformer(new ClassFileTransformer() {
     public byte[] transform(..., Injector injector) { if
(classBeingRetransformed == packageOwner) {
       injector.inject(bytes); return null;
     }}
    }, true);
    inst.retransformClasses(packageOwner);
  }
}

All you need is a class from the targeted package to inject a class of your
choosing. But of course, performance is terrible.

Again, it is really not my intention to be overly bothering with this
issue, I just think this is really important to get right. I have spent a
lot of time on eveluating the proposal and I do not find it sufficient. I
am saying this from experience in the field, I beg you to take me seriously
on this.

I also know that I am not the only agent vendor that is concered about this
API proposal and I will ask others to come forward as well.

Thanks again for your time and best regards,
Rafael

Am Fr., 5. Okt. 2018 um 15:53 Uhr schrieb Alan Bateman <
Alan.Bateman at oracle.com>:

> On 05/10/2018 13:17, Rafael Winterhalter wrote:
> > :
> >
> > However, for Java agents, there is still no good way to define
> > auxiliary classes. There is an open ticket on
> > https://bugs.openjdk.java.net/browse/JDK-8201784 and I have described
> > my concerns with the suggested API in the linked discussion as being
> > insufficient for many use cases. I still hope that a defineClass
> > method can be added to the Instrumentation interface; I would not know
> > of any other good use case that currently makes use of the internal
> > Unsafe version but agents.
> >
> > As of today, Unsafe::defineClass is however still a crucial element
> > for many Java agents that are widely used in the enterprise space,
> > e.g. for APM tools, security monitoring or metrics extraction. Not
> > offering an alternative would lock out these tools and stop a large
> > fraction of Java users from updating their production environments
> > until a new workaround is found. I hope that you consider an API for
> > Java agents to define classes as a blocker issue to be solved prior to
> > removing Unsafe::defineClass alltogether.
> >
> There was a lengthy thread about this topic on serviceability-dev
> earlier this year. As I'm sure you will remember, Mandy did propose
> additions to the Instrumentation API to support defining of auxiliary
> classes in the same runtime package as the instrumented class. The nice
> thing about that proposal is that extended the Instrumentation API in a
> way that is aligned with original purpose of this API. As I think we
> explained several times, the Instrumentation API is to support tools
> doing benign instrumentation. The Instrumentation API was never intended
> to support some of the scenarios that you brought up in the discussion.
> Yes, we understand that you are trying to support some "interesting"
> use-case, esp. around mocking, but many of these scenario that are way
> outside the scope of this API. We do need to get back to that discussion
> but it's important to set expectations at the same time.
>
> -Alan
>