Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[java] Fix intermittent NPE ClassSub.enclosingInfo is null #4834

Merged
merged 4 commits into from
Mar 11, 2024
Merged

[java] Fix intermittent NPE ClassSub.enclosingInfo is null #4834

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8c41a5
[java] Fix intermittent NPE ClassSub.enclosingInfo is null
adangel Feb 24, 2024
6b454de
[java] Improve logging in ParseLock in case of failure
adangel Feb 25, 2024
cf12435
[doc] Update release notes (#4757)
adangel Feb 25, 2024
e7ee97a
Merge branch 'master' into issue-4757-npe
jsotuyod Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ClassStub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,15 @@ protected void finishParse(boolean failed) {
: HashTreePSet.empty();
}

@Override
protected boolean canReenter() {
// We might call the parsing logic again in the same thread,
// 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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@Override
protected boolean postCondition() {
return signature != null && enclosingInfo != null;
Expand Down
2 changes: 1 addition & 1 deletion ...java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/GenericSigBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

void setSuperInterfaces(List<JClassType> supers) {
Expand Down
12 changes: 10 additions & 2 deletions pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ParseLock.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,22 @@ private ParseStatus getFinalStatus() {
try {
boolean success = doParse();
status = success ? ParseStatus.FULL : ParseStatus.FAILED;
this.status = status;
finishParse(!success);
} catch (Throwable t) {
status = ParseStatus.FAILED;
this.status = status;
LOG.error(t.toString(), t);
finishParse(true);
}

// the status must be updated as last statement, so that
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: a Thread.sleep() works better to reproduce the exceptions.

// other threads see the status FULL or FAILED only after finishParse()
// returns. Otherwise, some fields might not have been initialized.
//
// Note: the current thread might reenter the parsing logic through
// finishParse() -> ... -> ensureParsed(). In that case the status is still BEING_PARSED,
// and we don't call finishParse() again. See below for canReenter()
this.status = status;

assert status.isFinished : "Inconsistent status " + status;
assert postCondition() : "Post condition not satisfied after parsing sig " + this;
} else if (status == ParseStatus.BEING_PARSED && !canReenter()) {
Expand Down
28 changes: 28 additions & 0 deletions pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ClassStubTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html
*/

package net.sourceforge.pmd.lang.java.symbols.internal.asm;

import static org.junit.jupiter.api.Assertions.assertFalse;

import org.junit.jupiter.api.Test;
import org.pcollections.PSet;

import net.sourceforge.pmd.lang.java.JavaParsingHelper;
import net.sourceforge.pmd.lang.java.symbols.JClassSymbol;
import net.sourceforge.pmd.lang.java.types.TypeSystem;

class ClassStubTest {
// while parsing the annotation type, ClassStub's parseLock.ensureParsed()
// is called multiple times, reentering the parselock while the status is
// still BEING_PARSED.
@Test
void loadAndParseAnnotation() {
// class stub - annotation type
TypeSystem typeSystem = TypeSystem.usingClassLoaderClasspath(JavaParsingHelper.class.getClassLoader());
JClassSymbol classSymbol = typeSystem.getClassSymbol("java.lang.Deprecated");
PSet<String> annotationAttributeNames = classSymbol.getAnnotationAttributeNames();
assertFalse(annotationAttributeNames.isEmpty());
}
}