Spring/spring-tools
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

changed AST cache to NOT guard submitted work items against each other with synchronization, instead added synchronization to the AnnotationHierarchy class to focus synchronization on the code that hit some thread-safety bugs in the AST implementation

Browse Source
This commit is contained in:
Martin Lippert
2021-02-17 11:47:47 +01:00
parent 14efbf0e47
commit 84f9ea75d0
2 changed files with 82 additions and 86 deletions
Download Patch File Download Diff File Expand all files Collapse all files

92
headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/annotations/AnnotationHierarchies.java
View File

@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2017, 2018 Pivotal, Inc.
* Copyright (c) 2017, 2021 Pivotal, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
@@ -38,6 +38,10 @@ import com.google.common.collect.ImmutableList;
public abstract class AnnotationHierarchies {
private static final Logger log = LoggerFactory.getLogger(AnnotationHierarchies.class);
// this lock is used to protect multi-threaded access to this helper class
// due to https://bugs.eclipse.org/bugs/show_bug.cgi?id=571247
private static final Object lock = new Object();
protected static boolean ignoreAnnotation(String fqname) {
return fqname.startsWith("java."); //mostly intended to capture java.lang.annotation.* types. But really it should be
@@ -45,63 +49,73 @@ public abstract class AnnotationHierarchies {
};
public static Collection<ITypeBinding> getDirectSuperAnnotations(ITypeBinding typeBinding) {
try {
IAnnotationBinding[] annotations = typeBinding.getAnnotations();
if (annotations != null && annotations.length != 0) {
ImmutableList.Builder<ITypeBinding> superAnnotations = ImmutableList.builder();
for (IAnnotationBinding ab : annotations) {
ITypeBinding sa = ab.getAnnotationType();
if (sa != null) {
if (!ignoreAnnotation(sa.getQualifiedName())) {
superAnnotations.add(sa);
synchronized(lock) {
try {
IAnnotationBinding[] annotations = typeBinding.getAnnotations();
if (annotations != null && annotations.length != 0) {
ImmutableList.Builder<ITypeBinding> superAnnotations = ImmutableList.builder();
for (IAnnotationBinding ab : annotations) {
ITypeBinding sa = ab.getAnnotationType();
if (sa != null) {
if (!ignoreAnnotation(sa.getQualifiedName())) {
superAnnotations.add(sa);
}
}
}
return superAnnotations.build();
}
return superAnnotations.build();
}
catch (AbortCompilation e) {
log.debug("compilation aborted ", e);
// ignore this, it is most likely caused by broken source code, a broken classpath, or some optional dependencies not being on the classpath
}
return ImmutableList.of();
}
catch (AbortCompilation e) {
log.debug("compilation aborted ", e);
// ignore this, it is most likely caused by broken source code, a broken classpath, or some optional dependencies not being on the classpath
}
return ImmutableList.of();
}
public static Set<String> getTransitiveSuperAnnotations(ITypeBinding typeBinding) {
Set<String> seen = new HashSet<>();
findTransitiveSupers(typeBinding, seen).collect(Collectors.toList());
return seen;
synchronized(lock) {
Set<String> seen = new HashSet<>();
findTransitiveSupers(typeBinding, seen).collect(Collectors.toList());
return seen;
}
}
public static Stream<ITypeBinding> findTransitiveSupers(ITypeBinding typeBinding, Set<String> seen) {
String qname = typeBinding.getQualifiedName();
if (seen.add(qname)) {
return Stream.concat(
Stream.of(typeBinding),
getDirectSuperAnnotations(typeBinding).stream().flatMap(superBinding ->
findTransitiveSupers(superBinding, seen)
)
);
synchronized(lock) {
String qname = typeBinding.getQualifiedName();
if (seen.add(qname)) {
return Stream.concat(
Stream.of(typeBinding),
getDirectSuperAnnotations(typeBinding).stream().flatMap(superBinding ->
findTransitiveSupers(superBinding, seen)
)
);
}
return Stream.empty();
}
return Stream.empty();
}
public static boolean isSubtypeOf(Annotation annotation, String fqAnnotationTypeName) {
ITypeBinding annotationType = annotation.resolveTypeBinding();
if (annotationType!=null) {
return findTransitiveSupers(annotationType, new HashSet<>())
.anyMatch(superType -> superType.getQualifiedName().equals(fqAnnotationTypeName));
synchronized(lock) {
ITypeBinding annotationType = annotation.resolveTypeBinding();
if (annotationType!=null) {
return findTransitiveSupers(annotationType, new HashSet<>())
.anyMatch(superType -> superType.getQualifiedName().equals(fqAnnotationTypeName));
}
return false;
}
return false;
}
public static Collection<ITypeBinding> getMetaAnnotations(ITypeBinding actualAnnotation, Predicate<String> isKeyAnnotationName) {
Stream<ITypeBinding> allSupers = findTransitiveSupers(actualAnnotation, new HashSet<>())
.skip(1); //Don't include 'actualAnnotation' itself.
return allSupers
.filter(candidate -> isMetaAnnotation(candidate, isKeyAnnotationName))
.collect(CollectorUtil.toImmutableList());
synchronized(lock) {
Stream<ITypeBinding> allSupers = findTransitiveSupers(actualAnnotation, new HashSet<>())
.skip(1); //Don't include 'actualAnnotation' itself.
return allSupers
.filter(candidate -> isMetaAnnotation(candidate, isKeyAnnotationName))
.collect(CollectorUtil.toImmutableList());
}
}
private static boolean isMetaAnnotation(ITypeBinding candidate, Predicate<String> isKeyAnnotationName) {

76
headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/utils/CompilationUnitCache.java
View File

@@ -60,15 +60,11 @@ public final class CompilationUnitCache implements DocumentContentProvider {
private final ProjectObserver.Listener projectListener;
private final SimpleTextDocumentService documentService;
// private AsyncRunner async;
private final Cache<URI, CompilationUnit> uriToCu;
private final Cache<IJavaProject, Set<URI>> projectToDocs;
private final Cache<IJavaProject, Tuple2<List<Classpath>, INameEnvironmentWithProgress>> lookupEnvCache;
// private ReadLock readLock;
// private WriteLock writeLock;
public CompilationUnitCache(JavaProjectFinder projectFinder, SimpleLanguageServer server, ProjectObserver projectObserver) {
this.projectFinder = projectFinder;
this.projectObserver = projectObserver;
@@ -81,13 +77,7 @@ public final class CompilationUnitCache implements DocumentContentProvider {
this.projectToDocs = CacheBuilder.newBuilder().build();
this.lookupEnvCache = CacheBuilder.newBuilder().build();
// ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
// this.readLock = lock.readLock();
// this.writeLock = lock.writeLock();
this.documentService = server == null ? null : server.getTextDocumentService();
// this.async = server == null ? new AsyncRunner(Schedulers.single()) : server.getAsync();
// IMPORTANT ===> these notifications arrive within the lsp message loop, so reactions to them have to be fast
// and not be blocked by waiting for anything
@@ -108,44 +98,22 @@ public final class CompilationUnitCache implements DocumentContentProvider {
@Override
public void deleted(IJavaProject project) {
logger.info("CU Cache: deleted project {}", project.getElementName());
// async.execute(() -> {
// writeLock.lock();
// try {
invalidateProject(project);
// } finally {
// writeLock.unlock();
// }
// });
invalidateProject(project);
}
@Override
public void created(IJavaProject project) {
logger.info("CU Cache: created project {}", project.getElementName());
// async.execute(() -> {
// writeLock.lock();
// try {
invalidateProject(project);
// Load the new cache the value right away
loadLookupEnvTuple(project);
// } finally {
// writeLock.unlock();
// }
// });
invalidateProject(project);
loadLookupEnvTuple(project);
}
@Override
public void changed(IJavaProject project) {
logger.info("CU Cache: changed project {}", project.getElementName());
// async.execute(() -> {
// writeLock.lock();
// try {
invalidateProject(project);
// Load the new cache the value right away
loadLookupEnvTuple(project);
// } finally {
// writeLock.unlock();
// }
// });
invalidateProject(project);
// Load the new cache the value right away
loadLookupEnvTuple(project);
}
};
@@ -162,9 +130,13 @@ public final class CompilationUnitCache implements DocumentContentProvider {
}
/**
* Retrieves a CompiationUnitn AST from the cache and passes it to a requestor callback, applying
* proper thread synchronization around the requestor.
* <p>
* Never research shows at the AST is thread-safe when used in read-only mode:
* https://bugs.eclipse.org/bugs/show_bug.cgi?id=58314
*
* This means that the previous implemented synchronization around the requestor
* working on the AST is not necessary as long as the requestor operates in read-only
* mode on the AST nodes.
*
* Warning: Callers should take care to do all AST processing inside of the requestor callback and
* not pass of AST nodes to helper functions that work aynchronously or store AST nodes or ITypeBindings
* for later use. The JDT ASTs are not thread safe!
@@ -177,8 +149,20 @@ public final class CompilationUnitCache implements DocumentContentProvider {
return withCompilationUnit(project, uri, requestor);
}
/**
* Never research shows at the AST is thread-safe when used in read-only mode:
* https://bugs.eclipse.org/bugs/show_bug.cgi?id=58314
*
* This means that the previous implemented synchronization around the requestor
* working on the AST is not necessary as long as the requestor operates in read-only
* mode on the AST nodes.
*
* Warning: Callers should take care to do all AST processing inside of the requestor callback and
* not pass of AST nodes to helper functions that work aynchronously or store AST nodes or ITypeBindings
* for later use. The JDT ASTs are not thread safe!
*/
public <T> T withCompilationUnit(IJavaProject project, URI uri, Function<CompilationUnit, T> requestor) {
logger.info("CU Cache: work item for doc {}", uri.toString());
logger.info("CU Cache: work item submitted for doc {}", uri.toString());
if (project != null) {
@@ -206,16 +190,14 @@ public final class CompilationUnitCache implements DocumentContentProvider {
if (cu != null) {
try {
logger.info("CU Cache: sync start on AST for {}", uri.toString());
synchronized (cu.getAST()) {
return requestor.apply(cu);
}
logger.info("CU Cache: start work on AST for {}", uri.toString());
return requestor.apply(cu);
}
catch (Exception e) {
logger.error("", e);
}
finally {
logger.info("CU Cache: sync end on AST for {}", uri.toString());
logger.info("CU Cache: end work on AST for {}", uri.toString());
}
}
}