Bug 347110

Comment 1 Greg Wilkins 2011-05-25 03:43:14 EDT

Created attachment 196513 [details]
partial implementation

this implementation calls the transformers but does not do any doPrivileged wrapping, nor checking if the package is sealed etc.

Comment 2 Greg Wilkins 2011-08-29 01:17:14 EDT

There is still little demand for this... but if you do want it... please comment with your use-case to give us the reason to go forward and complete this.

Comment 3 Glyn Normington 2012-04-16 05:43:45 EDT

I think bug 376537 is the incorrect pre-req. Is there a typo in the bug number?

Comment 4 Eirik Lygre 2014-01-13 12:30:39 EST

Our use case is Spring, where we get the error message "ClassLoader [org.eclipse.jetty.webapp.WebAppClassLoader] does NOT provide an 'addTransformer(ClassFileTransformer)' method."

The workaround is simple enough: Add "-javaagent:spring-instrument-4.0.0.jar".

However, the combination of gradle and a number of different IDEs make this a bit cumbersome. IDEA 13 uses build.gradle as "the" build project, and you don't need to generate project files. However, you also have no really nice place to put the "-javaagent" definition, so you end up editing the project run configurations manually.

It would all be "just easier" if the WebAppClassLoader supported "addTransformer()"!

Comment 5 Eirik Lygre 2014-01-13 12:37:51 EST

The spring documentation even singles out Jetty as one of the special ones:

http://docs.spring.io/spring/docs/4.0.0.RELEASE/spring-framework-reference/htmlsingle/#aop-aj-ltw-environments:

"Generic Java applications

When class instrumentation is required in environments that do not support or are not supported by the existing LoadTimeWeaver implementations, a JDK agent can be the only solution. For such cases, Spring provides InstrumentationLoadTimeWeaver, which requires a Spring-specific (but very general) VM agent, org.springframework.instrument-{version}.jar (previously named spring-agent.jar).

To use it, you must start the virtual machine with the Spring agent, by supplying the following JVM options:

-javaagent:/path/to/org.springframework.instrument-{version}.jar
Note that this requires modification of the VM launch script which may prevent you from using this in application server environments (depending on your operation policies). Additionally, the JDK agent will instrument the entire VM which can prove expensive.

For performance reasons, it is recommended to use this configuration only if your target environment (such as Jetty) does not have (or does not support) a dedicated LTW."

Comment 6 Greg Wilkins 2014-01-19 17:56:42 EST

Eirik,

we'd be please to include in Jetty either direct support for addTransformer() or changes to make the -javaagent:spring-instrument-4.0.0.jar approach simpler to add.

However we have neither the time nor direct use-case experience to be able to do this on our own.

So could you look at WebAppClassLoader, the partial implementation done in 2011 and the current requirements of the spring LTW to suggest exactly what we need do to support addTransformer?    A patch would be most welcome, but failing that even a unit test that we would need to pass would be entirely helpful in getting this done and supported.

Comment 7 Eirik Lygre 2014-02-07 05:28:42 EST

I'll see what I can do to help :-)

This very first comment shows how we do it today, which is through a mechanism that is probably not acceptable for moving along. However, it works for our development purposes, and might be useful as input.

This implementation works like this:
- A patched version of java.net.URLClassLoader, really just to work around the fact that the various forms of URLClassLoader.defineClass() are all final, and can't be overridden (this fact increases the blood pressure)
- A patched version of jetty.WebAppClassLoader, overriding defineClass as required

I'll upload "everything" as a zip file; here is the meat of it all:

diff URLClassLoader.java URLClassLoader.java-170_51.txt
[removed comments, package and import diffs]
446c442
<             return doDefineClass(name, bb, cs);
---
>             return defineClass(name, bb, cs);
453c449
<             return doDefineClass(name, b, cs);
---
>             return defineClass(name, b, 0, b.length, cs);
457,467d452
<     protected Class<?> doDefineClass(String name, java.nio.ByteBuffer b,
<                                      CodeSource cs) {
<         return defineClass(name, b, cs);
<     }
<
<     protected Class<?> doDefineClass(String name,
<                                      byte[] b,
<                                      CodeSource cs) {
<         return defineClass(name, b, 0, b.length, cs);
<     }
<
[removed a javaNetAccess-diff that fails, but is not needed for us]



diff WebAppClassLoader.java WebAppClassLoader.jetty-910.txt
[removed comments, package and import diffs]
462,510d454
<
<
<     /* ------------------------------------------------------------ */
<
<     private java.util.concurrent.ConcurrentLinkedQueue<java.lang.instrument.ClassFileTransformer> classFileTransformers =
<             new java.util.concurrent.ConcurrentLinkedQueue<java.lang.instrument.ClassFileTransformer>();
<
<     public void addTransformer(final java.lang.instrument.ClassFileTransformer transformer) {
<         classFileTransformers.add(transformer);
<     }
<
<     @Override
<     protected Class<?> doDefineClass(String name, java.nio.ByteBuffer b,
<                                                CodeSource cs)
<     {
<         if (classFileTransformers.isEmpty())
<             return super.doDefineClass(name, b, cs);
<
<         return transformAndDefineClass(name, b.array(), cs);
<     }
<
<     @Override
<     protected final Class<?> doDefineClass(String name,
<                                                      byte[] b,
<                                                      CodeSource cs) {
<         System.out.println("transformAndDefineClass: name=" + name);
<
<         if (classFileTransformers.isEmpty())
<             return super.doDefineClass(name, b, cs);
<
<         return transformAndDefineClass(name, b, cs);
<     }
<
<     protected final Class<?> transformAndDefineClass(String name,
<                                            byte[] b,
<                                            CodeSource cs) {
<         System.out.println("transformAndDefineClass: name=" + name);
<
<         String transformName = name.replace('.', '/');
<         for (java.lang.instrument.ClassFileTransformer transformer : classFileTransformers) {
<             try {
<                 b = transformer.transform(this, transformName, null, null, b);
<             } catch (java.lang.instrument.IllegalClassFormatException e) {
<                 throw new RuntimeException (e);
<             }
<         }
<
<         return defineClass(name, b, 0, b.length, cs);
<     }

Comment 8 Eirik Lygre 2014-02-07 05:31:55 EST

Created attachment 239729 [details]
"Solution" based on patched java.net.URLClassLoader

Ref comment #7 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=347110#c7)

A "solution" based on patching java.net.URLClassLoader to expose defineClass() methods, and then using that inside the jetty WebAppClassLoader).

Comment 9 Eirik Lygre 2014-02-27 12:28:02 EST

Having gone another round on the original patch, I realize that my attempt was misguided, so to speak. The thought was that the internals of java.net.URLClassLoader (such as the URLClassPath) were very important, and that some ugly surgery was needed.

So, the original patch looks fair enough. Based on that, we created the following TransformingWebAppClassLoader which seems to work fine! I'll post back when we've run this solution in our environment for a couple of weeks.

package org.eclipse.jetty.webapp;

import org.eclipse.jetty.util.IO;

import java.io.IOException;
import java.io.InputStream;
import java.lang.instrument.ClassFileTransformer;
import java.lang.instrument.IllegalClassFormatException;
import java.net.URL;
import java.util.List;
import java.util.concurrent.CopyOnWriteArrayList;

/**
 *
 */
public class TransformingWebAppClassLoader extends WebAppClassLoader {

    public TransformingWebAppClassLoader(Context context) throws IOException {
        super(context);
    }

    public TransformingWebAppClassLoader(ClassLoader parent, Context context) throws IOException {
        super(parent, context);
    }

    /* ------------------------------------------------------------ */
    private final List<ClassFileTransformer> _transformers = new CopyOnWriteArrayList<ClassFileTransformer>();

    /* ------------------------------------------------------------ */
    public void addTransformer(ClassFileTransformer transformer)
    {
        _transformers.add(transformer);
    }

    /* ------------------------------------------------------------ */
    public boolean removeTransformer(ClassFileTransformer transformer)
    {
        return _transformers.remove(transformer);
    }

    /* ------------------------------------------------------------ */
    protected Class<?> findClass(final String name) throws ClassNotFoundException
    {
        Class<?> clazz=null;

        if (!_transformers.isEmpty())
        {
            String path = name.replace('.', '/').concat(".class");
            URL url = getResource(path);
            if (url==null)
                throw new ClassNotFoundException(name);

            InputStream content=null;
            try
            {
                content = url.openStream();
                byte[] bytes = IO.readBytes(content);

                for (ClassFileTransformer transformer : _transformers)
                    bytes=transformer.transform(this,name,null,null,bytes);

                clazz=defineClass(name,bytes,0,bytes.length);
            }
            catch (IOException e)
            {
                throw new ClassNotFoundException(name,e);
            }
            catch (IllegalClassFormatException e)
            {
                throw new ClassNotFoundException(name,e);
            }
            finally
            {
                if (content!=null)
                {
                    try
                    {
                        content.close();
                    }
                    catch (IOException e)
                    {
                        throw new ClassNotFoundException(name,e);
                    }
                }
            }
        }
        else
            clazz = super.findClass(name);

        return clazz;
    }
}

Comment 10 Greg Wilkins 2014-05-27 10:40:25 EDT

Eirik,

how is it going?  still working?

Comment 11 Eirik Lygre 2014-05-27 10:47:39 EDT

It is going very well. We have been running it in test since the last comment, and in a couple of smaller production systems since the beginning of the month. 

We're feeling very good about it, and would be very happy to see this integrated into the base WebAppClassLoader!

Comment 12 Greg Wilkins 2014-05-27 11:25:26 EDT

Almost 3 years to the day... committed for 9.2.1 in commit 19e1954

Comment 13 Greg Wilkins 2014-08-19 19:27:36 EDT

for Eirik Lygre <eirik.lygre@gmail.com> 

I don't know how to reopen an issue in bugzilla, so I'll at least start with this email.

We just got around to upgrading jetty, ready to remove our custom classloader, and notice that the patch does not satisfy the requirements by spring. In particular, the jetty-method has been named "addClassFileTransformer", while spring requires it to be called "addTransformer":

org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'loadTimeWeaver': Initialization of bean failed; nested exception is java.lang.IllegalStateException: ClassLoader [org.eclipse.jetty.webapp.WebAppClassLoader] does NOT provide an 'addTransformer(ClassFileTransformer)' method. Specify a custom LoadTimeWeaver or start your Java virtual machine with Spring's agent: -javaagent:org.springframework.instrument.jar          

I checked the bugzilla log, and see that this may not have been entirely clear. Anyway, I believe the major use case is spring, and I guess there are three different scenarios going forward:

* One scenario would be to just rename the methods in jetty's WebAppClassLoader, from { addClassFileTransformer, removeClassFileTransformer } to { addTransformer, removeTransformer }
* Another is to add new methods { addTransformer, removeTransformer } that just forwards to { addClassFileTransformer, removeClassFileTransformer }.
* Or, to leave the problem to Spring, and have their ReflectiveLoadTimeWeaver also handle the jetty naming.

Since this is a new feature in jetty, made mostly to make jetty work simpler with spring, I'd prefer one of the two first ones. The first is probably good enough, but if you have strict backward compatibility requirements, perhaps #2 must be it. With #2, you may also make { addTransformer, removeTransformer } the "real" methods, and deprecate { addClassFileTransformer, removeClassFileTransformer }.

Anyway, should I open a new bugzilla for this, or can you reopen the old one?

Eirik

Comment 14 Greg Wilkins 2014-08-19 19:35:22 EDT

Do you know if spring just wants the method signatures, or does it want the classloader to implement Instrumentation interface?

Comment 15 Greg Wilkins 2014-08-19 19:45:00 EDT

For now I have pushed commit 6b2b6c3 that deprecates the old names and uses the spring names.

Would be great to get a test from you to see if spring is happy with this or if anything else is needed.

Comment 16 Eirik Lygre 2014-08-20 02:28:43 EDT

First, spring, which uses only checks the method signature:

* Interface used inside spring: https://github.com/spring-projects/spring-framework/blob/master/spring-context/src/main/java/org/springframework/instrument/classloading/LoadTimeWeaver.java
* Implementation used inside spring: https://github.com/spring-projects/spring-framework/blob/master/spring-context/src/main/java/org/springframework/instrument/classloading/ReflectiveLoadTimeWeaver.java
* Spring will first try to create application server specific LoadTimeWeavers, based on class loader names, then fall back on the ReflectiveLoadTimeWeaver.

Partial code from ReflectiveLoadTimeWeaver:

public class ReflectiveLoadTimeWeaver implements LoadTimeWeaver {

  private static final String ADD_TRANSFORMER_METHOD_NAME = "addTransformer";

  public ReflectiveLoadTimeWeaver(ClassLoader classLoader) {
    this.classLoader = classLoader;
    this.addTransformerMethod = ClassUtils.getMethodIfAvailable(
      this.classLoader.getClass(),
      ADD_TRANSFORMER_METHOD_NAME,
      new Class<?>[] {ClassFileTransformer.class});
    // fail if not found
  }

  public void addTransformer(ClassFileTransformer transformer) {
    Assert.notNull(transformer, "Transformer must not be null");
    ReflectionUtils.invokeMethod(
      this.addTransformerMethod,
      this.classLoader,
      transformer);
  }
}

I may not be able to verify your implementation for a few weeks, due to vacation starting today, but here is the full source (minus constructor and whitespace) of the classloader that we currently use, which then triggers spring to happily use the ReflectiveLoadTimeWeaver above: 

public class TransformingWebAppClassLoader extends WebAppClassLoader {
  // Removed constructors, which just call parent, anyway
  public void addTransformer(ClassFileTransformer transformer)
  {
    super.addClassFileTransformer (transformer);
  }
  public boolean removeTransformer(ClassFileTransformer transformer)
  {
    return super.removeClassFileTransformer (transformer);
  }
}

With that, the commit at https://github.com/eclipse/jetty.project/commit/6b2b6c330db21ea09c4b22aa13ed9388d9d800d7 seems good.

Comment 17 Greg Wilkins 2014-08-26 19:47:19 EDT

Marking as fixed again.   Reopen if there is a problem.

thanks