Fine grain locking to avoid deadlocks in CU cache

Signed-off-by: aboyko <alex.boyko@broadcom.com>
This commit is contained in:
aboyko
2025-03-16 14:04:47 -04:00
parent e906ad9991
commit e0d45e2aa9
2 changed files with 67 additions and 43 deletions

View File

@@ -216,8 +216,6 @@ public final class CompilationUnitCache implements DocumentContentProvider {
logger.info("CU Cache: work item submitted for doc {}", uri.toASCIIString()); logger.info("CU Cache: work item submitted for doc {}", uri.toASCIIString());
if (project != null) { if (project != null) {
ReadLock lock = environmentCacheLock.readLock();
lock.lock();
try { try {
CompilationUnit cu = null; CompilationUnit cu = null;
try { try {
@@ -234,10 +232,16 @@ public final class CompilationUnitCache implements DocumentContentProvider {
} }
if (cu != null) { if (cu != null) {
projectToDocs.get(project.getLocationUri(), () -> new HashSet<>()).add(uri); ReadLock lock = environmentCacheLock.readLock();
lock.lock();
logger.debug("CU Cache: start work on AST for {}", uri.toString()); try {
logger.info("CU Cache: start work on AST for {}", uri.toString());
return requestor.apply(cu); return requestor.apply(cu);
} finally {
logger.info("CU Cache: end work on AST for {}", uri.toString());
lock.unlock();
}
} }
} }
catch (CancellationException e) { catch (CancellationException e) {
@@ -246,10 +250,6 @@ public final class CompilationUnitCache implements DocumentContentProvider {
catch (Exception e) { catch (Exception e) {
logger.error("", e); logger.error("", e);
} }
finally {
logger.debug("CU Cache: end work on AST for {}", uri.toString());
lock.unlock();
}
} }
return requestor.apply(null); return requestor.apply(null);
@@ -259,34 +259,49 @@ public final class CompilationUnitCache implements DocumentContentProvider {
CompletableFuture<CompilationUnit> cuFuture = uriToCu.getIfPresent(uri); CompletableFuture<CompilationUnit> cuFuture = uriToCu.getIfPresent(uri);
if (cuFuture == null) { if (cuFuture == null) {
cuFuture = CompletableFuture.supplyAsync(() -> { cuFuture = CompletableFuture.supplyAsync(() -> {
ReadLock lock = environmentCacheLock.readLock();
lock.lock();
logger.info("Started parsing CU for " + uri);
try { try {
logger.info("Started parsing CU for " + uri);
Tuple2<List<Classpath>, INameEnvironmentWithProgress> lookupEnvTuple = loadLookupEnvTuple(project); Tuple2<List<Classpath>, INameEnvironmentWithProgress> lookupEnvTuple = loadLookupEnvTuple(project);
String uriStr = uri.toASCIIString(); String uriStr = uri.toASCIIString();
String unitName = uriStr.substring(uriStr.lastIndexOf("/") + 1); // skip over '/' String unitName = uriStr.substring(uriStr.lastIndexOf("/") + 1); // skip over '/'
CompilationUnit cUnit = parse2(fetchContent(uri).toCharArray(), uriStr, unitName, lookupEnvTuple.getT1(), lookupEnvTuple.getT2(), CompilationUnit cUnit = parse2(fetchContent(uri).toCharArray(), uriStr, unitName, lookupEnvTuple.getT1(), lookupEnvTuple.getT2(),
annotationHierarchies.get(project.getLocationUri(), AnnotationHierarchies::new)); annotationHierarchies.get(project.getLocationUri(), AnnotationHierarchies::new));
logger.debug("CU Cache: created new AST for {}", uri.toASCIIString()); logger.debug("CU Cache: created new AST for {}", uri.toASCIIString());
logger.info("Parsed successfully CU for " + uri); logger.info("Parsed successfully CU for " + uri);
return cUnit; return cUnit;
} catch (Throwable t) { } catch (Throwable t) {
// Complete future exceptionally // Complete future exceptionally
throw new CompletionException(t); throw new CompletionException(t);
} finally {
logger.info("Finished parsing CU for {}", uri);
lock.unlock();
} }
}, createCuExecutorThreadPool); }, createCuExecutorThreadPool);
// Cache the future // Cache the future
uriToCu.put(uri, cuFuture); uriToCu.put(uri, cuFuture);
// If CU future completed exceptionally invalidate the cache entry // If CU future completed exceptionally invalidate the cache entry
cuFuture.exceptionally(t -> { cuFuture
if (!(t instanceof CancellationException)) { .thenAccept(cu -> {
logger.error("", t); synchronized(CompilationUnitCache.this) {
} try {
uriToCu.invalidate(uri); projectToDocs.get(project.getLocationUri(), () -> new HashSet<>()).add(uri);
return null; } catch (ExecutionException e) {
}); // shouldn't happen
}
}
})
.exceptionally(t -> {
if (!(t instanceof CancellationException)) {
logger.error("", t);
}
synchronized(CompilationUnitCache.this) {
uriToCu.invalidate(uri);
}
return null;
});
} }
return cuFuture; return cuFuture;
} }

View File

@@ -338,43 +338,52 @@ public class JdtLsProjectCache implements InitializableJavaProjectsService, Serv
private class JstLsClasspathListener implements ClasspathListener { private class JstLsClasspathListener implements ClasspathListener {
/*
* Synchronize to make non-reentrant such that events handled in predictable order
*/
@Override @Override
public void changed(Event event) { public synchronized void changed(Event event) {
log.debug("claspath event received {}", event); log.debug("claspath event received {}", event);
server.doOnInitialized(() -> { server.doOnInitialized(() -> {
try { try {
synchronized (table) { String uri = UriUtil.normalize(event.projectUri);
String uri = UriUtil.normalize(event.projectUri); log.debug("uri = {}", uri);
log.debug("uri = {}", uri); if (event.deleted) {
if (event.deleted) { log.debug("event.deleted = true");
log.debug("event.deleted = true"); IJavaProject deleted;
IJavaProject deleted = table.remove(uri); synchronized (table) {
if (deleted!=null) { deleted = table.remove(uri);
log.debug("removed from table = true"); }
notifyDelete(deleted); // Notify outside of the lock
} else { if (deleted!=null) {
log.warn("Deleted project not removed because uri {} not found in {}", uri, table.keySet()); log.debug("removed from table = true");
} notifyDelete(deleted);
} else { } else {
log.debug("deleted = false"); log.warn("Deleted project not removed because uri {} not found in {}", uri, table.keySet());
URI projectUri = new URI(uri); }
ClasspathData classpath = new ClasspathData(event.name, event.classpath.getEntries(), event.classpath.getJavaVersion()); } else {
IJavaProject oldProject = table.get(uri); log.debug("deleted = false");
URI projectUri = new URI(uri);
ClasspathData classpath = new ClasspathData(event.name, event.classpath.getEntries(), event.classpath.getJavaVersion());
IJavaProject oldProject, newProject;
synchronized(table) {
oldProject = table.get(uri);
if (oldProject != null && classpath.equals(oldProject.getClasspath())) { if (oldProject != null && classpath.equals(oldProject.getClasspath())) {
// nothing has changed // nothing has changed
return; return;
} }
IProjectBuild projectBuild = from(event.projectBuild); IProjectBuild projectBuild = from(event.projectBuild);
IJavaProject newProject = IS_JANDEX_INDEX newProject = IS_JANDEX_INDEX
? new JavaProject(getFileObserver(), projectUri, classpath, ? new JavaProject(getFileObserver(), projectUri, classpath,
JdtLsProjectCache.this, projectBuild) JdtLsProjectCache.this, projectBuild)
: new JdtLsJavaProject(server.getClient(), projectUri, classpath, JdtLsProjectCache.this, projectBuild); : new JdtLsJavaProject(server.getClient(), projectUri, classpath, JdtLsProjectCache.this, projectBuild);
table.put(uri, newProject); table.put(uri, newProject);
if (oldProject != null) { }
notifyChanged(newProject); // Notify outside of the lock
} else { if (oldProject != null) {
notifyCreated(newProject); notifyChanged(newProject);
} } else {
notifyCreated(newProject);
} }
} }
} catch (Exception e) { } catch (Exception e) {