JaCoCo fails to start when another agent uses UUID prior to JaCoCo

[nadavye]

This is a issue tracker. Please use our mailing list for general questions:
https://groups.google.com/forum/?fromgroups=#!forum/jacoco

Also check FAQ before opening an issue: http://www.jacoco.org/jacoco/trunk/doc/faq.html

Steps to reproduce

Create a javaagent which loads the UUID class and use that agent prior to JaCoCo.

Expected behaviour

JaCoCo should run normally

Actual behaviour

JaCoCo fails with an error similar to the following:

java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:483)
        at sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:386)
        at sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:401)
Caused by: java.lang.RuntimeException: Class java/util/UUID could not be instrumented.
        at org.jacoco.agent.rt.internal_6effb9e.core.runtime.ModifiedSystemClassRuntime.createFor(ModifiedSystemClassRuntime.java:138)
        at org.jacoco.agent.rt.internal_6effb9e.core.runtime.ModifiedSystemClassRuntime.createFor(ModifiedSystemClassRuntime.java:99)
        at org.jacoco.agent.rt.internal_6effb9e.PreMain.createRuntime(PreMain.java:55)
        at org.jacoco.agent.rt.internal_6effb9e.PreMain.premain(PreMain.java:47)
        ... 6 more
Caused by: java.lang.NoSuchFieldException: $jacocoAccess
        at java.lang.Class.getField(Class.java:1690)
        at org.jacoco.agent.rt.internal_6effb9e.core.runtime.ModifiedSystemClassRuntime.createFor(ModifiedSystemClassRuntime.java:136)
        ... 9 more
FATAL ERROR in native method: processing of -javaagent failed
Exception in thread "main"

[nadavye]

After reading the code, seems like you guys modifiy the UUID class and add the '$jacocoAccess' field to it and try to read it later. I assume that the problem is that JaCoCo is not supporting class-redefinition so if another agent loads the UUID, it doesn't get instrumented, thus, the '$jacocoAccess' field doesn't exist.

Thoughts?

[Godin]

@nadavye JVM loads and starts agents in order in which they are specified in command-line, so make sure that JaCoCo agent is a first one. Or please explain the reasons why you need another agent that uses UUID to be started prior to JaCoCo.

[Godin]

I assume that the problem is that JaCoCo is not supporting class-redefinition

Even if JaCoCo agent will be marked as Can-Redefine-Classes in terms of https://docs.oracle.com/javase/8/docs/api/java/lang/instrument/package-summary.html it won't help, because most JVMs do not currently support adding methods or fields to classes that are redefined (see http://openjdk.java.net/jeps/159). Such attempt on OpenJDK will result in java.lang.UnsupportedOperationException: class redefinition failed: attempted to change the schema (add/remove fields)

[nadavye]

Hi @Godin,

First, I would like to thank you for the quick answer.

Second, per your question, I have a Java Agent that runs along JaCoCo and it seems like one of its dependencies creates a UUID.

I understand what you are saying about the limitation of OpenJDK.
How do you suggest to solve it?
Maybe you guys can use another class as the host of the "$jacocoAccess" field? Can it be configurable?
Maybe you can add an API that will allow another JavaAgent to initialize JaCoCo by calling an init() method?

Note that I don't mind donating to the codebase - I just want that the solution will be aligned with your design & internal considerations.

[marchof]

@nadavye For some background information please see section "Coverage Runtime Dependency" in our architecture documentation.

I'm well aware that this is a hack which might cause problems in certain setups. But it is the best solution we found so far. Any new ideas are welcome!

Why does the workaround proposed by @Godin not work for you? Can't you put JaCoCo agent as the first agent on the command line?

[nadavye]

@marchof thanks for the link. I'll have a look at it.

Per @Godin's solution, note that our javaagent is being used by customers which run JaCoCo next to our agent. When they add our agent, we can't control the order, thus, from time to time we get a ticket about JaCoCo's failure.

What do you think about adding an API to JaCoCo that will register that field? This way I'll be include a small dependency in JaCoCo's API and invoke it when my agent initializes. This API will add the transformer that modifies UUID and if it fails, it will fail gracefully. This way, if JaCoCo will be loaded later, the '$jacocoAccess' field will found.

I know that's a hack, but it will allow our agents to co-exist.

[nadavye]

In the meanwhile, I'll try to think on more elegant solutions. I'll appreciate if you could also take some time on your end and see what can be done.

That being said - I appreciate your fast response time! :-) You guys are awesome 👍

[nadavye]

BTW - Is there a reason why you choose UUID?
Is that due to the fact that it is less likely to be loaded before the agent starts?
If so, is it possible to transform for example the 'ClassCastException' class? If so, we can add a parameter which allow to override the transformation of the UUID class with another class name, this could be awesome.

Thoughts?

[marchof]

@nadavye We picked UUID because this class seems not to be used with other agents. Originally it was java.lang.Void but we saw conflicts here with some application servers.

I'm strongly against a configuration option. Such technical options cause a lot of support questions.

What I could think of is using a list of classes which will tried one after the other. These classes should

1. be unlikely used by other agents
2. not cause any side effects while initialization

[Godin]

@nadavye IMO the simplest option is to tell to your users that JaCoCo should be the first one. This is consistent with what we tell to JaCoCo users 😉

And as @marchof I'm also against of configuration option for the same reasons. As well as against proposed API that is IMO more of a hack rather than a solution.

The list of classes is an interesting idea, even if there still will be possibility that all of them was loaded. We can improve error message, so to ease understanding that JaCoCo agent should be the first one.

[Godin]

@marchof As another idea: we use this field to cross boundaries of classloaders, so maybe instead agent can track classloaders for injection of a class into them that will be providing access for classes loaded by this classloader. Classes that are loaded by classloader into which we can't inject classes (one of main java classloader, don't remember name/term of it) believe already have a direct access to agent.

[marchof]

@Godin Some remarks to your ideas:

• Injecting of a runtime class: AFAIK it is required to make a protected method accessible. Is this possible with Jigsaw?
• The agent is on the application class loader, which is not visible from the bootstrap class loader.

[nadavye]

What about injecting code into each every class which derives of ClassLoader so it will load JaCoCo's classes from a certain location? Is that doable?

[nadavye]

@Godin - I prefer to resolve this issue rather then letting users fail and then tell them that due to limitation in JaCoCo it has to be first. That's bad experience for the customers and it will cause you guys to look bad and we don't want it ;-)

[Godin]

@marchof

AFAIK it is required to make a protected method accessible. Is this possible with Jigsaw?

Mocking frameworks have similar need, so there was a discussion about this and as far as I remember it ended in that JDK 9 will provide dedicated mechanism for such class injection. Will need to refresh my links and memory about that.

The agent is on the application class loader, which is not visible from the bootstrap class loader.

And bootstrap classloader is used for base java classes which are of a less interest for majority of our users. For those for whom this is of an interest, there is anyway difficulties such as computation of coverage is impossible for classes used during startup sequence. So maybe classes from bootstrap classloader shouldn't be our concern?

@nadavye

What about injecting code into each every class which derives of ClassLoader so it will load JaCoCo's classes from a certain location? Is that doable?

Isn't it just a rephrasing of my idea? And to answer whether this is doable or not, time to think (ref @marchof questions) and to experiment/prototype is required.

[nadavye]

@Godin - may bad. I thought that by "track classloaders" you meant that the agent will keep state. As long as we are on the same page, we are cool.

[marchof]

@Godin @nadavye

Until we find a better runtime access strategy (which I don't expect soon) I see the following realistic options to help our users:

1. Add FAQ entry
2. Improve error message
3. Work with list of classes

[nadavye]

@marchof
Sounds awesome.

Do you feel that we can modify this method in Premain.java:

private static IRuntime createRuntime(final Instrumentation inst) throws Exception {
		return ModifiedSystemClassRuntime.createFor(inst, "java/util/UUID");
}

To something like:

private static IRuntime createRuntime(final Instrumentation inst) throws Exception {
	String hostClassess[] = new String[]{"java/util/UUID", "foo", "bar"};
	for(String hostClass : hostClassess){
		try{
			return ModifiedSystemClassRuntime.createFor(inst, hostClass);
		}catch(RuntimeException e){
		}
	}			
	return null;
}

And then check inside the premain() method if the runtime is null. If that's the case we'll throw an exception that JaCoCo should be the first agent.

Thoughts?

[marchof]

@nadavye Yep, something like this. Maybe you can propose some classes which work with your setup?

[Godin]

For the record ClassCastException most likely is not a good candidate, because seems to be used during startup sequence:

$ cat <<END > App.java
class App {
  public static void main(String[] args) {
  }
}
END

$ javac App.java

$ java -verbose:class -javaagent:jacocoagent.jar App
[Loaded java.lang.ClassCastException from /home/godin/.java-select/versions/jdk1.8.0_131/jre/lib/rt.jar]
...
[Loaded org.jacoco.agent.rt.internal_0d87adf.PreMain from file:/tmp/jacocoagent.jar]

[Godin]

@marchof to my taste potential candidates are:

• java/lang/UnknownError (since JDK 1.0)
• java/util/UnknownFormatFlagsException (since JDK 1.5)

[marchof]

@Godin Nice! For every candidate we should run a little test to see what other classes are loaded when it gets initialized. Just to make sure we don't cause side-effects on our side.

[Godin]

@marchof

$ cat <<END > App.java
class App {
  public static void main(String[] args) {
    if (args.length > 0)
      new java.lang.UnknownError();
  }
}
END

$ javac App.java

$ java -verbose:class App with 2>&1 > with.txt

$ java -verbose:class App 2>&1 > without.txt

$ diff with.txt without.txt

gives:

391d390
< [Loaded java.lang.UnknownError from /home/godin/.java-select/versions/jdk1.8.0_131/jre/lib/rt.jar]

Also nothing suspicious and heavy in source code of java/lang/UnknownError and its superclasses. So I think it is very good candidate for replacement of java/util/UUID without list of candidates, especially that it supposed to be used solely by Virtual Machine and in rare situations. Our tests pass after such replacement.

Given that such replacement is complimentary to improvement of error message - do you have a proposal/idea of a message?

[Godin]

As a remark: there seems to be a backdoor as another agent can create field $jacocoAccess, but I was planning to close this in #532.

[Godin]

@nadavye BTW bear in mind that even in case of changes on our side, we can't do anything for older versions, so that with them error still will be there as well as recommendation to change order. I don't know what your agent is doing, but maybe on your side you can delay activity of your agent (especially with class java/util/UUID).

[nadavye]

@Godin - Thanks for the clarification. I'm aware of it.
BTW - Do you have any estimation about this fix? Do you want me to change the class name and issue a PR?

[marchof]

@Godin

The exception message I would reword as follows:

JaCoCo runtime cannot be initialized with class %s.

For the FAQ I would add the following entry:

Why does the JaCoCo agent fail with a exception message during startup?

The JaCoCo agent hooks into the Java runtime which might fail under the following conditions:

• If JaCoCo is combined with other agents these agents might interfere with the the JaCoCo runtime. In this case make sure the JaCoCo agent is the first one specified on the command line.
• Avoid to specify multiple JaCoCo agents for the same JVM. Especially specifying multiple JaCoCo agents in different versions will fail.
• If you use IBM's J9 JRE please set the VM option -Xshareclasses:none.

[Godin]

For the record - tiny script to assist with possible candidates:

unzip -l src.zip | awk '{print $4}' | sed 's/.java//g' | grep 'java/' | sort > available.txt
java -verbose:class | awk '{print $2}' | tr '.' '/' | grep 'java/' | sort > startup.txt
diff startup.txt available.txt

[Godin]

@marchof I was thinking about more user-friendly message, but maybe such would lead to confusion given the various possible reasons. Leaving aside for now update of message and FAQ, could you please review #555 ?

@nadavye could you please also test that #555 solves problem for you?

[nadavye]

@Godin
Just loaded my agent with -verbose:class and I can confirm that java/lang/UnknownError is not loaded so I assume it should be fine.

Which version contains this fix?

@Godin & @marchof
Thanks for your quick response and fix. You guys rock ;-)

[Godin]

@nadavye

Just loaded my agent with -verbose:class and I can confirm that java/lang/UnknownError is not loaded so I assume it should be fine.

To be 100% sure you'd better check with 0.7.10-SNAPSHOT version of JaCoCo that can be obtained from snapshot repository - http://www.jacoco.org/jacoco/trunk/doc/repo.html

Which version contains this fix?

All the changes that were merged into the master and their respective versions as always are reflected on page http://www.jacoco.org/jacoco/trunk/doc/changes.html
For every pull request the AppVeyor builds provide the corresponding all-in-one zip for download - http://www.jacoco.org/jacoco/trunk/doc/environment.html

[nadavye]

Hi Godin,
When do you plan to include it in the formal release of JaCoCo?

[nadavye]

@Godin @marchof - seems like that the issue was resolved. Thanks!
When it should be deployed as an official version?

[Godin]

@nadavye please stop repeating question. It will be released when we'll finish work on version. And there is not only your ticket. Thank you for your understanding.

[nadavye]

Sorry, I've thought that you didn't see it as I didn't tag you in the first post. My bad :\ Didn't mean to pressure.

…

On Tue, Oct 24, 2017 at 11:25 PM, Evgeny Mandrikov ***@***.*** > wrote: @nadavye <https://github.com/nadavye> please stop repeating question. It will be released when we'll finish work on version. And there is not only your ticket. Thank you for your understanding. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#551 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOZ71MrFvqI7Uq5o_lR-Gw_5eIY2XNFbks5svkeegaJpZM4OAmjJ> .

-- Kind regards, Nadav <https://www.sealights.io/regression-test-code-coverage/>

[Charlie7381]

Hi,

I saw the same error, wondering whether this get fixed by 0.7.10.201712151226 release or not?

Thank you!

[Godin]

@Charlie7381 please carefully read this thread, specifically - #551 (comment) and #551 (comment) , an you'll be able to answer your question by yourself.

[Godin]

I consider that #555 in 0.8.0 resolves main story of this ticket. If needed future improvements should be done via separate tickets.