Check that PROBE_FIELD_NAME is not null when using it. #771
Conversation
|
Hi @LaurentTho3 thanks for looking at this. Under most circumstances this sort of approach would not be acceptable as it is not thread safe, however in this case I think the worst consequence would be that we would loose some coverage as the array would be overwritten. That would be an improvement on erroring with a NullPointerException. If you have time to investigate can you see if you can figure out why is not called for #770 and #746? A solution that robustly ensured this happened would be preferable. |
|
Hello @hcoles , Sorry, I did not think about the thread-safety aspect of things. I do not completely understand the situation, but from what I understand, the problem is not that clinit is not called, but that it is not called first. This post seems to be related: https://stackoverflow.com/q/11419519 . The problem would then occur when a class has a static member of its own type I think. Looking at the (instrumented) bytecode generated for com/example/NestedClassTest$MyEnum$1 in #770 would confirm this: Bytecode// class version 52.0 (52) // access flags 0x4030 final enum com/example/NestedClassTest$MyEnum$1 extends com/example/NestedClassTest$MyEnum {OUTERCLASS com/example/NestedClassTest$MyEnum null // access flags 0x1019 // access flags 0x1019 // access flags 0x0 // access flags 0x8 Here Similarly, looking at the Type class which was one of the classes giving errors in #746 (https://github.com/apache/commons-bcel/blob/rel/commons-bcel-6.4.0/src/main/java/org/apache/bcel/generic/Type.java) we see the following static member: There might be other cases where this problem might occur, but I do not know which ones. A solution might be to somehow ensure that clinit comes first as it seems that the initialisers are run "in textual order", but that might create other problems. Otherwise, I do not see another workaround for this problem, although I will keep thinking about it. |
|
I'm now wondering if there is a hole in the static initalizer approach when classes are loaded by reflection with initalize = false passed in. Perhaps your lazy initialization route is the right way to go, but we'd need to handle the thread safety concern. I don't think a synchronized block on every probe retrieval would be acceptable, but we could possibly follow the double checked locking pattern. https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html This would require that the field was volatile though, and I have a memory of that being an issue. There are at least some scenarios where I think the jvm spec says that it can't be (interfaces?). Might be that we need different strategies for different types of class. |
|
Loading classes with reflection and initialize = false would cause the same problem I guess, yes. The java specs say that "It is a compile-time error if a final variable is also declared volatile." (https://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.3.1.4), and also that "Every field declaration in the body of an interface is implicitly public, static, and final" (https://docs.oracle.com/javase/specs/jls/se8/html/jls-9.html#jls-9.2), so it looks like volatile and interfaces would not mix. Could we make the field not final as we are generating synthetic bytecode? I have started playing a bit with the double checked locking pattern. I use the following code for the synchronised code but it fails in MutationCoverageReportSystemTest.shouldMutateClassesSuppliedToAlternateClassPath, I will try to think of a better way. |
|
Reading up on lazy initialisation, and I was wondering if the on demand holder idiom https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom could be a solution? At least it does not require volatile, but maybe breaks other things I did not think about. |
|
Think what is required is a two strategy approach. For interfaces things should work exactly as they are with constructor initialization and final fields. For classes + enums the probe field should be volatile and use double checked locking. I don't see why you'd need to lookup the type, something like if (probes == null) {
probes = initProbes();
}
private synchronized boolean[] initProbes() {
if (probes != null) {
return probes;
}
return new probes[howevermany];
}Should work. |
|
Tried my hand at the double locking but it fails in PitMojoIT.shouldWorkWithGWTMockito because of the following error: I really do not get why this error is popping up from the changes I made, and it seems to not be happening on all jdk versions. Maybe I am missing something obvious? |
|
Not clear on what is happening here. The jdk version dependent behaviour I do understand - gtwmockito doesn't work with all versions, so the test only runs for those it is supported for. Had a bit of a hack locally with your branch. The method init probes method ought to be marked private and synthetic (so that reflection frameworks don't pick it up). Changing this didn't make gwtmockito any happier. On a hunch I hacked it so that the probes were always initialized in the static initialiser for classes as well as interfaces. This did work, although i don't really understand why. So that seems to be a working solution, but given the lazy initialisation will only be used in corner cases it doesn't feel great. An alternative would be to dig into why the initializer wasn't called in the two issues that resulted in null pointers, and add whatever code is required for those scenarios (plus check whether class.forname with initialize false passed in does cause and issue). |
|
@LaurentTho3 Finally understand what's going on here. The issue is that parent classes are always initilialized before their children. So given something like public class ParentChildInitializationTest {
@Test
public void test() {
new TesteeChild();
}
}
class TesteeParent {
static TesteeChild child = new TesteeChild();
}
class TesteeChild extends TesteeParent {
final static Object f = "hello";
TesteeChild() {
System.out.println(f);
}
}You get output like so the array probe field in child classes like this are not initialized when their init method is called from the parent static initializer. Knowing this, I think the appropriate fix is your initial null check, but with the static initializer left in place. In 99% of cases the probe will be created as normal, for the corner case it will be created by the init call from the parent static initializer. I've put this together in #850. Thanks for your work on this, I suspect it has been blowing up in a lot of codebases without being reported. |
Hello,
This is a fix for #770, which also seems to solve #746.
It looks like in some configurations, it is possible that we try to access the coverage probes field ($$pitCoverageProbes) before it is initialised in the static initialiser (). This leads to a NullPointerException.
Here, instead of initialising the field in the static initialiser, we check if it is null every time we access it and initialise it if needed. All the tests are passing.