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.

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

[Bug]: Under multithreading, using the isOverriding method will lead to incorrect results of the getElements method #5619

Closed
honghao12 opened this issue Jan 15, 2024 · 0 comments · Fixed by #5621
Labels

Comments

@honghao12
Copy link

Describe the bug

Under multithreading, using the isOverriding method will lead to incorrect results of the getElements(new TypeFilter<>(CtExecutableReference.class)) method;
This bug is sporadic, executed multiple times and only shows up sometimes

Source code you are trying to analyze/transform

https://github.com/apache/tomcat/blob/main/java/org/apache/jasper/compiler/Node.java

Source code for your Spoon processing

import com.google.common.collect.Lists;
import spoon.Launcher;
import spoon.reflect.CtModel;
import spoon.reflect.declaration.CtMethod;
import spoon.reflect.declaration.CtType;
import spoon.reflect.reference.CtExecutableReference;
import spoon.reflect.reference.CtTypeReference;
import spoon.reflect.visitor.filter.TypeFilter;

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.CompletableFuture;

public class Test1 {
    public static void main(String[] args) {
        Launcher launcher = new Launcher();
        launcher.getEnvironment().setComplianceLevel(14);
        launcher.getEnvironment().setAutoImports(true);
        launcher.getEnvironment().setIgnoreDuplicateDeclarations(true);
        launcher.addInputResource("tomcat-main\\java\\org\\apache\\jasper\\compiler\\Node.java");
        CtModel model = launcher.buildModel();

        List<CompletableFuture<Void>> futureList = new ArrayList<>();
        List<CtMethod> ctMethodList = model.getElements(new TypeFilter<>(CtMethod.class));
        List<List<CtMethod>> ctMethodPartLists = Lists.partition(ctMethodList, 10);
        for (List<CtMethod> sublist : ctMethodPartLists) {
            futureList.add(CompletableFuture.runAsync(() -> {
                for (CtMethod ctMethod : sublist) {
                    List<CtExecutableReference<?>> ctExecutableReferenceList = ctMethod.getElements(
                        new TypeFilter<>(CtExecutableReference.class));
                    String name = ctMethod.getPath().toString();
                    if("#subPackage[name=org]#subPackage[name=apache]#subPackage[name=jasper]#subPackage[name=compiler]#containedType[name=Node]#nestedType[name=Visitor]#method[signature=visit(org.apache.jasper.compiler.Node$AttributeDirective)]".equals(name)) {
                         if(1 != ctExecutableReferenceList.size()){
                            System.out.println(name + " expect:" + 1 + " actual:" + ctExecutableReferenceList.size());
                        } else {
                            System.out.println(name + " expect:" + 1 + " actual:" + ctExecutableReferenceList.size());
                        }
                    }

                    CtType<?> srcType = ctMethod.getDeclaringType();
                    CtTypeReference<?> superclassReference = srcType.getSuperclass();
                    if (Objects.nonNull(superclassReference)) {
                        CtType<?> superclass = superclassReference.getTypeDeclaration();
                        if (Objects.nonNull(superclass)) {
                            Set<CtMethod<?>> dstMethods = superclass.getMethods();
                            for (CtMethod<?> dstMethod : dstMethods) {
                                //In multithreading, using the isOverriding method causes ctMethod.getElements(new TypeFilter<>(CtExecutableReference.class)) is not correct
                                if (ctMethod.isOverriding(dstMethod)) {

                                }
                            }
                        }
                    }
                }
            }));
        }

        CompletableFuture<Void> allFutures = CompletableFuture.allOf(futureList.toArray(CompletableFuture[]::new));
        allFutures.join();
    }
}

Actual output

ctExecutableReference size expect:1 actual:0

Expected output

ctExecutableReference size expect:1 actual:1

Spoon Version

10.4.3-beta-2

JVM Version

11

What operating system are you using?

win

@honghao12 honghao12 added the bug label Jan 15, 2024
MartinWitt added a commit that referenced this issue Jan 17, 2024
Synchronization logic has been added to the method adaptation process in the TypeAdaptor class to prevent multi-threading issues. This change was necessary because without synchronization, simultaneous modifications by multiple threads could lead to inconsistent or unexpected results. The relevant issue can be found at #5619.
I-Al-Istannen pushed a commit that referenced this issue Jan 17, 2024
When checking whether a method overrides another, we need to adapt both
methods to a common ground.
In an early version this required cloning the entire method, including
its body, to perform the adaption. PR #4862 fixed this by nulling the
body before cloning the method and then restoring it afterwards. This
operation is not thread safe, as it may write concurrently and also
modifies what other parallel threads might see when traversing the
model.

This patch now introduces a private override specifying whether we are
only interested in the signature. If that is the case, we explicitly
construct a new method using the factory instead of cloning the
original.

Closes: #5619
I-Al-Istannen added a commit that referenced this issue Jan 17, 2024
When checking whether a method overrides another, we need to adapt both
methods to a common ground.
In an early version this required cloning the entire method, including
its body, to perform the adaption. PR #4862 fixed this by nulling the
body before cloning the method and then restoring it afterwards. This
operation is not thread safe, as it may write concurrently and also
modifies what other parallel threads might see when traversing the
model.

This patch now introduces a private override specifying whether we are
only interested in the signature. If that is the case, we explicitly
construct a new method using the factory instead of cloning the
original.

Closes: #5619
SirYwell pushed a commit that referenced this issue Jan 22, 2024
When checking whether a method overrides another, we need to adapt both
methods to a common ground.
In an early version this required cloning the entire method, including
its body, to perform the adaption. PR #4862 fixed this by nulling the
body before cloning the method and then restoring it afterwards. This
operation is not thread safe, as it may write concurrently and also
modifies what other parallel threads might see when traversing the
model.

This patch now introduces a private override specifying whether we are
only interested in the signature. If that is the case, we explicitly
construct a new method using the factory instead of cloning the
original.

Closes: #5619

Co-authored-by: I-Al-Istannen <i-al-istannen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant