diff --git a/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/annotations/AnnotationHierarchies.java b/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/annotations/AnnotationHierarchies.java index 5cdab5338..420343506 100644 --- a/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/annotations/AnnotationHierarchies.java +++ b/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/annotations/AnnotationHierarchies.java @@ -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 getDirectSuperAnnotations(ITypeBinding typeBinding) { - try { - IAnnotationBinding[] annotations = typeBinding.getAnnotations(); - if (annotations != null && annotations.length != 0) { - ImmutableList.Builder 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 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 getTransitiveSuperAnnotations(ITypeBinding typeBinding) { - Set seen = new HashSet<>(); - findTransitiveSupers(typeBinding, seen).collect(Collectors.toList()); - return seen; + synchronized(lock) { + Set seen = new HashSet<>(); + findTransitiveSupers(typeBinding, seen).collect(Collectors.toList()); + return seen; + } } public static Stream findTransitiveSupers(ITypeBinding typeBinding, Set 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 getMetaAnnotations(ITypeBinding actualAnnotation, Predicate isKeyAnnotationName) { - Stream 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 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 isKeyAnnotationName) { diff --git a/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/utils/CompilationUnitCache.java b/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/utils/CompilationUnitCache.java index c994e2439..0cdcda2c0 100644 --- a/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/utils/CompilationUnitCache.java +++ b/headless-services/spring-boot-language-server/src/main/java/org/springframework/ide/vscode/boot/java/utils/CompilationUnitCache.java @@ -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 uriToCu; private final Cache> projectToDocs; private final Cache, 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. - *

+ * 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 withCompilationUnit(IJavaProject project, URI uri, Function 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()); } } }