OverlappingFileLockException in JaCoCo when using offline instrumentation due to duplicate shutdown hooks (classloading issue) #9837
Comments
|
cc @harthorst & @tkalmar |
|
WDYT @stuartwdouglas (and @geoand maybe)? |
|
I think we want the agent based instrumentation to be our default approach. I am not sure why it says that this approach is not compatible with Quarkus, its the approach we use for code coverage within Quarkus itself. |
|
@stuartwdouglas It is because of constructor injection. The problem is described here: https://quarkus.io/guides/tests-with-coverage#the-coverage-does-not-seem-to-correspond-to-the-reality |
|
What if Quarkus would create no-arg constructors for CDI-beans by default? cc @mkouba @manovotn More context: We considered https://projectlombok.org/api/lombok/NoArgsConstructor.html but we do not want to pull in Lombok just because of that and with this annotation we still would have to remember to add it to new (CDI-managed) classes. That discussion lead to the following solutions:
Plan B (way inferior to the pervious solutions):
|
|
That is the problem, it means the classes loaded into memory are not the same as the ones jacoco sees on disk, and this causes problems. We do have the ability to dump the transformed classes to disk, I wonder if jacoco could use that. |
AFAIK, as long as the classes already have the constructor when they are loaded (from whatever source) and are processed by the JaCoCo agent, eveything is fine. But I guess this is too early for the Quarkus mechanism? |
|
On Tue, 9 Jun 2020 at 21:24, Falko Modler ***@***.***> wrote:
are not the same as the ones jacoco sees on disk,
AFAIK, as long as the classes already have the constructor when they are
*loaded* (from whatever source) and are processed by the JaCoCo agent,
eveything is fine. But I guess this is too early for the Quarkus mechanism?
They definitely have them when they are loaded, the agent will see the
final class file that will be provided to the JVM. It won't be the same as
the one on disk though.
Maybe it is also loading the originals, and this is confusing it.
Stuart
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9837 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACQG65W2YWMOWLFM5TVKT3RVYLWFANCNFSM4NVIWV3Q>
.
|
|
Again AFAIK but in online mode the JaCoCo agent should not do anything with the class files. cc @marchof |
|
@famod The JaCoCo agent also transforms class files, but on-the-fly while classes are loaded in memory. During report generation the exact same original class files are required (source files are optional if you want highlighted class files). If the class files are transformed before JaCoCo agent sees them, they do not match any more. JaCoCo does not look at specific constructors. Any modification will result in a mismatch. This is correctly described in another chapter of the document referenced earlier: https://quarkus.io/guides/tests-with-coverage#instrumenting-the-classes-instead |
|
@marchof thanks for clarifying this. So how do we me move on from here? |
|
To quote myself:
I've just learned that you cannot modify existing code with annotation processors, you can only generate new classes. |
|
One possibility would be that we could maybe write a jacoco extension that integrates with @QuarkusTest, and somehow we wire it up so it works of the transformed classes. Not sure how practical this idea is (or even if it is possible) but maybe it would also allow for the best user experience. |
|
Update regarding offline instrumentation: I've just successfully tested a fix that is supposed to land in JaCoCo 0.8.6: jacoco/jacoco#1057 Once 0.8.6 is out I am going to update the Quarkus docs (and the example) to use 0.8.6. Aside from that I'd still prefer a working online instrumentation. |
|
We also have that issue and apparently others. See e.g. https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/JaCoCo.20coverage.20and.20JDK.20versions |
|
@famod So you built JaCoCo 0.8.6 locally with that change? Or is there a snapshot version already out in some repo? |
|
@pilhuhn I had built this branch: jacoco/jacoco#1057 See also: jacoco/jacoco#331 (comment) |
|
🎉 Update regarding online instrumentation: It seems we (or better to say @mickroll who actually implemented it) were successful in creating a plugin for byte-buddy-maven-plugin that auto-adds no-arg constructors to classes with https://gist.github.com/mickroll/1310eeed7f34a514461b25a470d434db Please let me or @mickroll know, whether you need more info (preferably as a comment under the gist). |
|
@stuartwdouglas WDYT, should we update the documentation to point to this issue and maybe also the JaCoCo issue? |
|
Is there a reproducer somewhere for the agent based instrumentation issue? |
|
@stuartwdouglas I've just modified the quickstart:
target/site/jacoco* will then not contain any coverage for PS: I kept the separation into UTs and ITs, which makes it a bit more complex. |
|
So it looks like this is hard coded into the maven plugin, so we can't override it to look at the transformed classes. The best way forward might be to create a Quarkus jacoco extension. We could perform the instrumentation using our existing transformers, and do the reporting based on the transformed classes. |
Bummer, 0.8.6 has just been released without this fix: jacoco/jacoco#331 (comment) 😞 Update, quote from one of the jacoco members:
|
|
I had a bit of a think about how to improve this, and started some PoC work here: https://github.com/quarkusio/quarkus/compare/master...stuartwdouglas:jacoco?expand=1 My idea is to have a Jacoco extension, that will use our transformers to instrument, and then automatically generate a report once the tests are done. There are some challenges though, as there is no reliable way to automatically link to the source. |
|
@stuartwdouglas sounds interesting! Btw, we have been using our ByteBuddy plugin for weeks now, without any major problems. |
|
Is there any update here? |
|
I have come back to this, and have figured out how to create a Jacoco extension: https://github.com/stuartwdouglas/quarkus/tree/jacoco Basically all you need to do is add the extension, and Jacoco will be wired up automatically, including report generation, with no plugins etc required in the pom. |
|
The linked PR should fix the offline instrumentation issue, while the new subsystem should allow for online instrumentation. |
ghost
added this to the 
|
For the record, as this is only solved for "offline" mode:
|
Describe the bug
We are trying to implement test coverage measurement as described here: https://quarkus.io/guides/tests-with-coverage#instrumenting-the-classes-instead
For the most part this is working, but we are seeing this exception at the end of the surefire/test executions in some of our Maven modules:
After we stumbled upon jacoco/jacoco#331 (comment), I debugged this and indeed in some cases the JaCoCo Agent ShutdownHook is registered twice, most likely because some of our instrumented classes are loaded from different classloaders (by Quarkus).
In one case the first
Agent.getInstance()call stack looks like this:Notes:
SharedConfigSourceProvideris a customConfigSourceProviderwe had to implement because we have multipleapplication*.yamlfiles in multiple modulesThe second
Agent.getInstance()call stack looks like this:It seems our
SharedConfigSourceProvideris loaded a second time by a different classloader and thereforeAgent.singletonisnull(not initialized yet).In this case there is only a single
QuarkusTestin the respective module, no other test is present.In another case the first
Agent.getInstance()is triggered by a non-QuarkusTest (vanilla Junit5 Jupiter test) and the call stack looks like this:Notes:
ConfigurationContextis one of our classesConfigurationContextTest.doBefore()is a@BeforeAllmethodAfter this non-QuarkusTest there is a QuarkustTest which triggers another "initial"
Agent.getInstance()invocation through ArC:Note:
SoapClientAccessorBuilderis one of our classes.This can all be debugged by adding a breakpoint to
Runtime.addShutdownHook()plusmvn ... -Dmaven.surefire.debug.Expected behavior
Quarkus classloading should not interfere with JaCoCo coverage via offline instrumentation.
Actual behavior
Duplicate JaCoCo Shutdownhook registrations seem to be caused by Quarkus classloading mechanisms, causing
OverlappingFileLockExceptionsto be logged that leave the user uncertain whether the coverage data can be trusted.To Reproduce
Steps to reproduce the behavior for the
ConfigSourceProvidercase:This is basically https://github.com/quarkusio/quarkus-quickstarts/tree/1.5.0.Final/tests-with-coverage-quickstart with a no-op
ConfigSourceProvider.ConfigSourceProvidermvn clean verifyshould yield:I can also try to create a reproducer for the other case, if needed.
Configuration
n/a
Environment (please complete the following information):
uname -aorver:MINGW64_NT-10.0-18363 W4DEUMSY9003463 3.0.7-338.x86_64 2019-11-21 23:07 UTC x86_64 Msysjava -version:1.4.2.Final&1.5.0.Finalmvnw --versionorgradlew --version):Apache Maven 3.6.3Additional context
This might be more of an interoperability issue between Quarkus and JaCoCo than a "classical" Quarkus-only bug, but since JaCoCo offline instumentation is "officially" documented in the Quarkus docs this should work without issues.
The classes indirectly causing the duplicate hooks can be excluded from JaCoCo instrumentation, but then they will (obviously) have 0% coverage in the report.
In one we case we did do so, though, because the "offending" class was a
QuarkusTestResourceLifecycleManagerfrom a test support module which should be ignored anyway.The text was updated successfully, but these errors were encountered: