Skip to content

Commit

Permalink
Add marker diagnostics asynchronously
Browse files Browse the repository at this point in the history 
Making it synchronous can block LS streams and cause deadlocks
  • Loading branch information
@mickaelistria
mickaelistria committed Mar 17, 2023
1 parent f894c17 commit 112fa33
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 64 deletions.
58 changes: 45 additions & 13 deletions org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/diagnostics/DiagnosticsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
package org.eclipse.lsp4e.test.diagnostics;

import static org.eclipse.lsp4e.test.TestUtils.waitForAndAssertCondition;
import static org.junit.Assert.*;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import java.io.File;
import java.io.FileOutputStream;
Expand Down Expand Up @@ -51,7 +53,9 @@
import org.eclipse.swt.graphics.Font;
import org.eclipse.swt.graphics.FontData;
import org.eclipse.swt.graphics.RGB;
import org.eclipse.swt.widgets.Display;
import org.eclipse.ui.ide.IDE;
import org.eclipse.ui.tests.harness.util.DisplayHelper;
import org.eclipse.ui.texteditor.MarkerUtilities;
import org.junit.Before;
import org.junit.Rule;
Expand Down Expand Up @@ -82,10 +86,17 @@ public void testDiagnostics() throws CoreException {

diagnosticsToMarkers.accept(new PublishDiagnosticsParams(file.getLocationURI().toString(), diagnostics));

assertTrue(DisplayHelper.waitForCondition(Display.getCurrent(), 2000, () -> {
try {
return file.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, false, IResource.DEPTH_INFINITE).length
== diagnostics.size();
} catch (CoreException e) {
return false;
}
}));

IMarker[] markers = file.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, false,
IResource.DEPTH_INFINITE);
assertEquals(diagnostics.size(), markers.length);

for (int i = 0; i < diagnostics.size(); i++) {
Diagnostic diagnostic = diagnostics.get(i);
IMarker marker = markers[i];
Expand All @@ -99,8 +110,14 @@ public void testDiagnostics() throws CoreException {

diagnosticsToMarkers.accept(new PublishDiagnosticsParams(file.getLocationURI().toString(), Collections.emptyList()));

markers = file.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, false, IResource.DEPTH_INFINITE);
assertEquals(0, markers.length);
assertTrue(DisplayHelper.waitForCondition(Display.getCurrent(), 2000, () -> {
try {
return file.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, false, IResource.DEPTH_INFINITE).length
== 0;
} catch (CoreException e) {
return false;
}
}));
}

@Test
Expand All @@ -117,8 +134,10 @@ public void testFileBuffersNotLeaked() throws Exception {

diagnosticsToMarkers.accept(new PublishDiagnosticsParams(file.getLocationURI().toString(), diagnostics));

IDocument shouldHaveBeenClosed = LSPEclipseUtils.getExistingDocument(file);
assertNull(shouldHaveBeenClosed);
assertTrue(DisplayHelper.waitForCondition(Display.getDefault(), 2000, () -> {
IDocument shouldHaveBeenClosed = LSPEclipseUtils.getExistingDocument(file);
return shouldHaveBeenClosed == null;
}));


}
Expand All @@ -134,10 +153,17 @@ public void testDiagnosticsRangeAfterDocument() throws CoreException {

diagnosticsToMarkers.accept(new PublishDiagnosticsParams(file.getLocationURI().toString(), diagnostics));

assertTrue(DisplayHelper.waitForCondition(Display.getCurrent(), 2000, () -> {
try {
return file.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, false,
IResource.DEPTH_INFINITE).length == diagnostics.size();
} catch (CoreException e) {
return false;
}
}));

IMarker[] markers = file.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, false,
IResource.DEPTH_INFINITE);
assertEquals(diagnostics.size(), markers.length);

for (int i = 0; i < diagnostics.size(); i++) {
Diagnostic diagnostic = diagnostics.get(i);
IMarker marker = markers[i];
Expand All @@ -160,9 +186,15 @@ public void testDiagnosticsFromVariousLS() throws Exception {
IMarker[] markers = file.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, true, IResource.DEPTH_ZERO);
assertEquals("no marker should be shown at file initialization", 0, markers.length);
TestUtils.openEditor(file);
Thread.sleep(300); //give some time to LSs to process
markers = file.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, true, IResource.DEPTH_ZERO);
assertEquals("there should be 1 marker for each language server", 2, markers.length);

assertTrue("there should be 1 marker for each language server", DisplayHelper.waitForCondition(Display.getCurrent(), 50000000, () -> {
try {
return file.findMarkers(LSPDiagnosticsToMarkers.LS_DIAGNOSTIC_MARKER_TYPE, true,
IResource.DEPTH_ZERO).length == 2;
} catch (CoreException e) {
return false;
}
}));
}

@Test
Expand Down Expand Up @@ -257,7 +289,7 @@ private void confirmResourceChanges(IFile file, Diagnostic diagnostic, int expec
}
return false;
});
assertEquals(expectedChanges, redrawCountListener.getAsInt());
waitForAndAssertCondition(5_000, () -> expectedChanges == redrawCountListener.getAsInt());
} finally {
workspace.removeResourceChangeListener(redrawCountListener);
}
Expand Down
104 changes: 53 additions & 51 deletions org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/diagnostics/LSPDiagnosticsToMarkers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@
import org.eclipse.core.resources.IMarker;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.resources.IWorkspace;
import org.eclipse.core.resources.IWorkspaceRunnable;
import org.eclipse.core.resources.WorkspaceJob;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.NullProgressMonitor;
import org.eclipse.core.runtime.Status;
import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.jface.text.BadLocationException;
Expand Down Expand Up @@ -128,63 +130,63 @@ private void updateEditorAnnotations(@NonNull ISourceViewer sourceViewer, Publis
}
}

private void updateMarkers(PublishDiagnosticsParams diagnostics, IResource resource) throws CoreException {
final var toDeleteMarkers = new HashSet<IMarker>(
Arrays.asList(resource.findMarkers(markerType, false, IResource.DEPTH_ONE)));
toDeleteMarkers
.removeIf(marker -> !Objects.equals(marker.getAttribute(LANGUAGE_SERVER_ID, ""), languageServerId)); //$NON-NLS-1$
final var newDiagnostics = new ArrayList<Diagnostic>();
final var toUpdate = new HashMap<IMarker, Diagnostic>();
private WorkspaceJob updateMarkers(PublishDiagnosticsParams diagnostics, IResource resource) throws CoreException {
WorkspaceJob job = new WorkspaceJob("Update markers from diagnostics") { //$NON-NLS-1$

// A language server can scan the whole project and generate diagnostics for files that are not currently open in the IDE
// (the markers will show up in the problem view). If so, need to open the document temporarily but be sure to release it
// when we're done
IDocument existingDocument = LSPEclipseUtils.getExistingDocument(resource);
final boolean hasDiagnostics = !diagnostics.getDiagnostics().isEmpty();
final boolean temporaryLoadDocument = existingDocument == null;
IDocument document = (hasDiagnostics && temporaryLoadDocument) ? LSPEclipseUtils.getDocument(resource): existingDocument;
for (Diagnostic diagnostic : diagnostics.getDiagnostics()) {
IMarker associatedMarker = getExistingMarkerFor(document, diagnostic, toDeleteMarkers);
if (associatedMarker == null) {
newDiagnostics.add(diagnostic);
} else {
toDeleteMarkers.remove(associatedMarker);
toUpdate.put(associatedMarker, diagnostic);
}
}
IWorkspaceRunnable runnable = monitor -> {
try {
for (Diagnostic diagnostic : newDiagnostics) {
Map<String, Object> markerAttributes = computeMarkerAttributes(document, diagnostic, resource);
resource.createMarker(markerType, markerAttributes);
}
for (Entry<IMarker, Diagnostic> entry : toUpdate.entrySet()) {
Map<String, Object> markerAttributes = computeMarkerAttributes(document, entry.getValue(), resource);
updateMarker(markerAttributes, entry.getKey());
}
toDeleteMarkers.forEach(t -> {
try {
t.delete();
} catch (CoreException e) {
LanguageServerPlugin.logError(e);
@Override
public IStatus runInWorkspace(IProgressMonitor monitor) throws CoreException {
final var toDeleteMarkers = new HashSet<IMarker>(
Arrays.asList(resource.findMarkers(markerType, false, IResource.DEPTH_ONE)));
toDeleteMarkers
.removeIf(marker -> !Objects.equals(marker.getAttribute(LANGUAGE_SERVER_ID, ""), languageServerId)); //$NON-NLS-1$
final var newDiagnostics = new ArrayList<Diagnostic>();
final var toUpdate = new HashMap<IMarker, Diagnostic>();

// A language server can scan the whole project and generate diagnostics for files that are not currently open in the IDE
// (the markers will show up in the problem view). If so, need to open the document temporarily but be sure to release it
// when we're done
IDocument existingDocument = LSPEclipseUtils.getExistingDocument(resource);
final boolean hasDiagnostics = !diagnostics.getDiagnostics().isEmpty();
final boolean temporaryLoadDocument = existingDocument == null;
IDocument document = (hasDiagnostics && temporaryLoadDocument) ? LSPEclipseUtils.getDocument(resource): existingDocument;
for (Diagnostic diagnostic : diagnostics.getDiagnostics()) {
IMarker associatedMarker = getExistingMarkerFor(document, diagnostic, toDeleteMarkers);
if (associatedMarker == null) {
newDiagnostics.add(diagnostic);
} else {
toDeleteMarkers.remove(associatedMarker);
toUpdate.put(associatedMarker, diagnostic);
}
});
} catch (CoreException e) {
if (resource.isAccessible()) {
LanguageServerPlugin.logError(e);
}
} finally {
if (document != null && temporaryLoadDocument) {
try {

try {
for (Diagnostic diagnostic : newDiagnostics) {
Map<String, Object> markerAttributes = computeMarkerAttributes(document, diagnostic, resource);
resource.createMarker(markerType, markerAttributes);
}
for (Entry<IMarker, Diagnostic> entry : toUpdate.entrySet()) {
Map<String, Object> markerAttributes = computeMarkerAttributes(document, entry.getValue(), resource);
updateMarker(markerAttributes, entry.getKey());
}
toDeleteMarkers.forEach(t -> {
try {
t.delete();
} catch (CoreException e) {
LanguageServerPlugin.logError(e);
}
});
} finally {
if (document != null && temporaryLoadDocument) {
FileBuffers.getTextFileBufferManager().disconnect(resource.getFullPath(), LocationKind.IFILE, new NullProgressMonitor());
} catch (CoreException e) {
LanguageServerPlugin.logError(e);
}
}
return Status.OK_STATUS;
}
};
IWorkspace ws = resource.getWorkspace();
ws.run(runnable, ws.getRuleFactory().markerRule(resource), IWorkspace.AVOID_UPDATE, new NullProgressMonitor());
job.setSystem(true);
job.setRule(resource);
job.schedule();
return job;
}

protected void updateMarker(@NonNull Map<String, Object> targetAttributes, @NonNull IMarker marker) {
Expand Down

0 comments on commit 112fa33

Please sign in to comment.