From 84f9ea75d0cc4126ab3b39d6542b80e7097768b0 Mon Sep 17 00:00:00 2001 From: Martin Lippert Date: Wed, 17 Feb 2021 11:47:47 +0100 Subject: [PATCH] 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 --- .../annotations/AnnotationHierarchies.java | 92 +++++++++++-------- .../boot/java/utils/CompilationUnitCache.java | 76 ++++++--------- 2 files changed, 82 insertions(+), 86 deletions(-) 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()); } } }