[java] Fix intermittent NPE ClassSub.enclosingInfo is null #4834
Conversation
| LOG.error(t.toString(), t); | ||
| finishParse(true); | ||
| } | ||
|
|
||
| // the status must be updated as last statement, so that |
There was a problem hiding this comment.
I think, this is the reason why other threads might see ClassStub.enclosingInfo as null - it is set by finishParse() (at least for simple top level classes), but finishParse() was called after the status was set FULL (finished). That means, another thread wouldn't have waited until finishParse() is finished... I could reproduce this problem by adding a Thread.yield() or a Thread.sleep() at the beginning of finishParse(), just before enclosingInfo is set.
There was a problem hiding this comment.
Update: a Thread.sleep() works better to reproduce the exceptions.
| // e.g. in order to determine "annotAttributes", getDeclaredMethods() is called, which | ||
| // calls ensureParsed(). | ||
| // Note: Other threads can't reenter, since our thread own the ParseLock monitor. | ||
| return true; |
There was a problem hiding this comment.
That's a consequence that we set the status of the ParseLock to FULL later - finishParse() actually calls back into ensureParsed() and tries to do the parsing again. Since we are in the same thread, we already own the monitor and actually reenter the ParseLock - but the status is already "BEING_PARSED".
I think, for ClassStub it's ok to allow reenter without being picky. The only field, that is being finished in finishParse is annotAttributes - everything else is already in set.
| @@ -163,7 +163,7 @@ protected boolean isGeneric() { | |||
|
|
|||
| @Override | |||
| protected boolean postCondition() { | |||
| return (superItfs != null && superType != null || signature == null) && typeParameters != null; | |||
| return superItfs != null && (superType != null || signature == null) && typeParameters != null; | |||
There was a problem hiding this comment.
I didn't figure out yet, why superItfs seems to end up as null. As far as I see, it is always initialized by the signature parser.
The superType can indeed be null - e.g. if we have java.lang.Object - and that can only happen, if signature == null. So I think, the parentheses are wrong. But I doubt it makes a big difference.
There was a problem hiding this comment.
superItfs ends up being null, if the doParse() call is aborted with a NPE - then we never set superItfs or superType.
I've also seen the postCondition failed when I reproduced the bug with Thread.sleep.
Generated by 🚫 Danger |
| LOG.error(t.toString(), t); | ||
| finishParse(true); | ||
| } | ||
|
|
||
| // the status must be updated as last statement, so that |
There was a problem hiding this comment.
Update: a Thread.sleep() works better to reproduce the exceptions.
| finishParse(!success); | ||
| } catch (Throwable t) { | ||
| status = ParseStatus.FAILED; | ||
| this.status = status; | ||
| LOG.error(t.toString(), t); | ||
| LOG.error("Parsing failed in ParseLock#doParse()", t); |
There was a problem hiding this comment.
Figured out the 2nd stacktrace of #4757 now, the case when GenericSigBase#supertIfs is null:
- We are parsing a signature of some kind
- This runs into a NPE, because ClassStub#enclosingInfo is null (because we updated ParseLock#status too early)
- This NPE is caught here - and that means, that doParse() didn't finish correctly
- In doParse() we would set the super interfaces
- But two lines earlier in SignatureParser -
- we call typeParamsWrapper() that eventually calls
- ClassStub#getEnclosingTypeParameterOwner()
- And that's where the NPE happens
So, the issue that superIfs is null, is a consequential error of the earlier NPE and both should be fixed now.
At least, I couldn't reproduce them anymore with Thread.sleep.
Describe the PR
Partial fix for #4757
ClassStub.enclosingInfo might indeed be null, when other threads want to use it. This is the case for stacktraces 1a, 1b and the first in 2.
Not yet solved: why is GenericSigBase.LazyClassSignature#superItfs null - which causes the NPE in TypeOps.mapPreservingSelf(TypeOps.java:951)...
Related issues
Ready?
./mvnw clean verifypasses (checked automatically by github actions)