diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 3c14674217..3e50f0fc86 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -17,6 +17,8 @@ This is a {{ site.pmd.release_type }} release. ### 🐛 Fixed Issues * ant * [#1860](https://github.com/pmd/pmd/issues/1860): \[ant] Reflective access warnings on java > 9 and java < 17 +* java + * [#5293](https://github.com/pmd/pmd/issues/5293): \[java] Deadlock when executing PMD in multiple threads ### 🚨 API Changes diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ClassStub.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ClassStub.java index 6a050ca5b3..15ae7b0535 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ClassStub.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ClassStub.java @@ -84,12 +84,7 @@ final class ClassStub implements JClassSymbol, AsmStub, AnnotationOwner { this.resolver = resolver; this.names = new Names(internalName); - this.parseLock = new ParseLock() { - // note to devs: to debug the parsing logic you might have - // to replace the implementation of toString temporarily, - // otherwise an IDE could call toString just to show the item - // in the debugger view (which could cause parsing of the class file). - + this.parseLock = new ParseLock("ClassStub:" + internalName) { @Override protected boolean doParse() throws IOException { try (InputStream instream = loader.getInputStream()) { @@ -315,9 +310,9 @@ final class ClassStub implements JClassSymbol, AsmStub, AnnotationOwner { } @Override - public boolean isGeneric() { + public int getTypeParameterCount() { parseLock.ensureParsed(); - return signature.isGeneric(); + return signature.getTypeParameterCount(); } @Override diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/GenericSigBase.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/GenericSigBase.java index 6784468646..eabeb13c5a 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/GenericSigBase.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/GenericSigBase.java @@ -46,9 +46,9 @@ abstract class GenericSigBase { protected List typeParameters; private final ParseLock lock; - protected GenericSigBase(T ctx) { + protected GenericSigBase(T ctx, String parseLockName) { this.ctx = ctx; - this.lock = new ParseLock() { + this.lock = new ParseLock(parseLockName) { @Override protected boolean doParse() { GenericSigBase.this.doParse(); @@ -81,7 +81,11 @@ abstract class GenericSigBase { protected abstract boolean postCondition(); - protected abstract boolean isGeneric(); + protected abstract int getTypeParameterCount(); + + protected boolean isGeneric() { + return getTypeParameterCount() > 0; + } public void setTypeParams(List tvars) { assert this.typeParameters == null : "Type params were already parsed for " + this; @@ -105,6 +109,7 @@ abstract class GenericSigBase { private static final String OBJECT_BOUND = ":" + OBJECT_SIG; private final @Nullable String signature; + private final int typeParameterCount; private @Nullable JClassType superType; private List superItfs; @@ -116,8 +121,9 @@ abstract class GenericSigBase { @Nullable String signature, // null if doesn't use generics in header @Nullable String superInternalName, // null if this is the Object class String[] interfaces) { - super(ctx); + super(ctx, "LazyClassSignature:" + ctx.getInternalName() + "[" + signature + "]"); this.signature = signature; + this.typeParameterCount = GenericTypeParameterCounter.determineTypeParameterCount(this.signature); this.rawItfs = CollectionUtil.map(interfaces, ctx.getResolver()::resolveFromInternalNameCannotFail); this.rawSuper = ctx.getResolver().resolveFromInternalNameCannotFail(superInternalName); @@ -157,8 +163,9 @@ abstract class GenericSigBase { } @Override - protected boolean isGeneric() { - return signature != null && TypeParamsParser.hasTypeParams(signature); + protected int getTypeParameterCount() { + // note: no ensureParsed() needed, the type parameters are counted eagerly + return typeParameterCount; } @Override @@ -206,6 +213,7 @@ abstract class GenericSigBase { static class LazyMethodType extends GenericSigBase implements TypeAnnotationReceiver { private final @NonNull String signature; + private final int typeParameterCount; private @Nullable TypeAnnotationSet receiverAnnotations; private List parameterTypes; @@ -233,8 +241,9 @@ abstract class GenericSigBase { @Nullable String genericSig, @Nullable String[] exceptions, boolean skipFirstParam) { - super(ctx); + super(ctx, "LazyMethodType:" + (genericSig != null ? genericSig : descriptor)); this.signature = genericSig != null ? genericSig : descriptor; + this.typeParameterCount = GenericTypeParameterCounter.determineTypeParameterCount(genericSig); // generic signatures already omit the synthetic param this.skipFirstParam = skipFirstParam && genericSig == null; this.rawExceptions = exceptions; @@ -288,8 +297,9 @@ abstract class GenericSigBase { @Override - protected boolean isGeneric() { - return TypeParamsParser.hasTypeParams(signature); + protected int getTypeParameterCount() { + // note: no ensureParsed() needed, the type parameters are counted eagerly + return typeParameterCount; } void setParameterTypes(List params) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/GenericTypeParameterCounter.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/GenericTypeParameterCounter.java new file mode 100644 index 0000000000..28f309c43b --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/GenericTypeParameterCounter.java @@ -0,0 +1,36 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.symbols.internal.asm; + +import org.objectweb.asm.signature.SignatureReader; +import org.objectweb.asm.signature.SignatureVisitor; + +class GenericTypeParameterCounter extends SignatureVisitor { + private int count; + + GenericTypeParameterCounter() { + super(AsmSymbolResolver.ASM_API_V); + } + + @Override + public void visitFormalTypeParameter(String name) { + count++; + } + + public int getCount() { + return count; + } + + static int determineTypeParameterCount(String signature) { + if (signature == null) { + return 0; + } + + SignatureReader signatureReader = new SignatureReader(signature); + GenericTypeParameterCounter counter = new GenericTypeParameterCounter(); + signatureReader.accept(counter); + return counter.getCount(); + } +} diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ModuleStub.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ModuleStub.java index 3db93664d3..1c0b33b0d2 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ModuleStub.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ModuleStub.java @@ -35,7 +35,7 @@ class ModuleStub implements JModuleSymbol, AsmStub, AnnotationOwner { this.resolver = resolver; this.moduleName = moduleName; - this.parseLock = new ParseLock() { + this.parseLock = new ParseLock("ModuleStub:" + moduleName) { @Override protected boolean doParse() throws IOException { try (InputStream instream = loader.getInputStream()) { diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ParseLock.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ParseLock.java index a7bfada5ea..a0150e00fe 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ParseLock.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/symbols/internal/asm/ParseLock.java @@ -17,15 +17,30 @@ abstract class ParseLock { private static final Logger LOG = LoggerFactory.getLogger(ParseLock.class); private volatile ParseStatus status = ParseStatus.NOT_PARSED; + private final String name; + + protected ParseLock(String name) { + this.name = name; + } public void ensureParsed() { getFinalStatus(); } + private void logParseLockTrace(String prefix) { + if (LOG.isTraceEnabled()) { + LOG.trace("{} {}: {}", Thread.currentThread().getName(), String.format("%-15s", prefix), this); + } + } + private ParseStatus getFinalStatus() { ParseStatus status = this.status; if (!status.isFinished) { + logParseLockTrace("waiting on"); + synchronized (this) { + logParseLockTrace("locked"); + status = this.status; if (status == ParseStatus.NOT_PARSED) { this.status = ParseStatus.BEING_PARSED; @@ -54,6 +69,7 @@ abstract class ParseLock { throw new IllegalStateException("Thread is reentering the parse lock"); } } + logParseLockTrace("released"); } return status; } @@ -85,7 +101,7 @@ abstract class ParseLock { @Override public String toString() { - return "ParseLock{status=" + status + '}'; + return "ParseLock{name=" + name + ",status=" + status + '}'; } private enum ParseStatus { diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symbols/DeadlockTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symbols/DeadlockTest.java new file mode 100644 index 0000000000..15358be481 --- /dev/null +++ b/pmd-java/src/test/java/net/sourceforge/pmd/lang/java/symbols/DeadlockTest.java @@ -0,0 +1,157 @@ +/* + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.symbols; + +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +import net.sourceforge.pmd.lang.java.JavaParsingHelper; +import net.sourceforge.pmd.lang.java.ast.ASTClassType; +import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit; + +/** + * Tests to help analyze [java] Deadlock when executing PMD in multiple threads #5293. + * + * @see [java] Deadlock when executing PMD in multiple threads #5293 + */ +class DeadlockTest { + + abstract static class Outer implements GenericInterface, GenericClass> { + // must be a nested class, that is reusing the type param T of the outer class + abstract static class Inner { + Inner(Outer grid) { } + } + } + + static class GenericBaseClass { } + + interface GenericInterface { } + + abstract static class GenericClass extends GenericBaseClass> { } + + @Test + @Timeout(2) + void parseWithoutDeadlock() throws InterruptedException { + /* + Deadlock: + t1 -> locks parse for Outer.Inner and waits for parse lock for Outer + ├─ t1 waiting on : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer$Inner[Ljava/lang/Object;],status=NOT_PARSED} + └─ t1 locked : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer$Inner[Ljava/lang/Object;],status=NOT_PARSED} + └─ t1 waiting on : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer[Ljava/lang/Object;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericInterface;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericClass;>;],status=BEING_PARSED} + t2 -> locks parse for Outer, locks parse for GenericInterface and then waits for parse lock for Outer.Inner + ├─ t2 waiting on : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer[Ljava/lang/Object;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericInterface;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericClass;>;],status=NOT_PARSED} + └─ t2 locked : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer[Ljava/lang/Object;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericInterface;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericClass;>;],status=NOT_PARSED} + ├─ t2 waiting on : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest[null],status=NOT_PARSED} + ├─ t2 locked : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest[null],status=NOT_PARSED} + ├─ t2 released : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest[null],status=FULL} + ├─ t2 waiting on : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer[Ljava/lang/Object;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericInterface;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericClass;>;],status=BEING_PARSED} + ├─ t2 locked : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer[Ljava/lang/Object;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericInterface;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericClass;>;],status=BEING_PARSED} + ├─ t2 released : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer[Ljava/lang/Object;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericInterface;Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericClass;>;],status=BEING_PARSED} + └─ t2 waiting on : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericClass[Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericBaseClass;>;],status=NOT_PARSED} + ├─ t2 locked : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericClass[Lnet/sourceforge/pmd/lang/java/symbols/DeadlockTest$GenericBaseClass;>;],status=NOT_PARSED} + └─ t2 waiting on : ParseLock{name=LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer$Inner[Ljava/lang/Object;],status=NOT_PARSED} + + + In order to reproduce the deadlock reliably, add the following piece into ParseLock, just at the beginning + of the synchronized block (line 42): + + // t1 needs to wait after having the lock, so that t2 can go on and wait on the same lock + if (Thread.currentThread().getName().equals("t1") && this.toString().contains("LazyClassSignature:net/sourceforge/pmd/lang/java/symbols/DeadlockTest$Outer$Inner[ exceptions = new ArrayList<>(); + Thread.UncaughtExceptionHandler exceptionHandler = (t, e) -> { + exceptions.add(e); + e.printStackTrace(); + }; + + Thread t1 = new Thread(() -> { + ASTCompilationUnit class1 = JavaParsingHelper.DEFAULT.parse( + "package net.sourceforge.pmd.lang.java.symbols;\n" + + "import net.sourceforge.pmd.lang.java.symbols.DeadlockTest.Outer;\n" + + " class Class1 {\n" + + " public static Outer.Inner newInner(Outer grid) {\n" + + " return null;\n" + + " }\n" + + " }\n" + ); + assertNotNull(class1); + + // Outer.Inner = return type of method "newInner" + List classTypes = class1.descendants(ASTClassType.class).toList(); + ASTClassType outerInner = classTypes.get(0); + assertGenericClassType(outerInner, "Inner", "X", "T"); + + // Outer = qualifier of Outer.Inner + ASTClassType outer = classTypes.get(1); + assertEquals("Outer", outer.getSimpleName()); + assertNull(outer.getTypeArguments()); + + // Outer = formal parameter type of method newInner + ASTClassType outerFormalParam = classTypes.get(3); + assertGenericClassType(outerFormalParam, "Outer", "X", "T"); + }, "t1"); + t1.setUncaughtExceptionHandler(exceptionHandler); + + Thread t2 = new Thread(() -> { + ASTCompilationUnit class2 = JavaParsingHelper.DEFAULT.parse( + "package net.sourceforge.pmd.lang.java.symbols;\n" + + "import net.sourceforge.pmd.lang.java.symbols.DeadlockTest.Outer;\n" + + " class Class2 {\n" + + " protected Outer theOuter;\n" + + " }\n" + ); + assertNotNull(class2); + + // Outer = type of field "theOuter" + ASTClassType firstClassType = class2.descendants(ASTClassType.class).first(); + assertNotNull(firstClassType); + assertGenericClassType(firstClassType, "Outer", "M", "T"); + }, "t2"); + t2.setUncaughtExceptionHandler(exceptionHandler); + + t1.start(); + t2.start(); + + t1.join(); + t2.join(); + + assertAll(exceptions.stream() + .map(e -> () -> { + throw e; + })); + } + + private static void assertGenericClassType(ASTClassType classType, String simpleName, String actualTypeParamName, String originalTypeParamName) { + assertEquals(simpleName, classType.getSimpleName()); + assertEquals(1, classType.getTypeArguments().size()); + assertEquals(actualTypeParamName, ((ASTClassType) classType.getTypeArguments().get(0)).getSimpleName()); + JTypeParameterOwnerSymbol symbol = (JTypeParameterOwnerSymbol) classType.getTypeMirror().getSymbol(); + assertEquals(1, symbol.getTypeParameterCount()); + assertEquals(originalTypeParamName, symbol.getTypeParameters().get(0).getName()); + } +} diff --git a/pmd-java/src/test/resources/simplelogger.properties b/pmd-java/src/test/resources/simplelogger.properties new file mode 100644 index 0000000000..8a4eafe283 --- /dev/null +++ b/pmd-java/src/test/resources/simplelogger.properties @@ -0,0 +1,5 @@ +# +# BSD-style license; for more info see http://pmd.sourceforge.net/license.html +# + +#org.slf4j.simpleLogger.log.net.sourceforge.pmd.lang.java.symbols.internal.asm.ParseLock=trace