From 112fa3394a45626c8d008ebcbf368fcb6d5ff6a8 Mon Sep 17 00:00:00 2001 From: Mickael Istria Date: Thu, 16 Mar 2023 11:49:45 +0100 Subject: [PATCH] Add marker diagnostics asynchronously Making it synchronous can block LS streams and cause deadlocks --- .../test/diagnostics/DiagnosticsTest.java | 58 +++++++--- .../diagnostics/LSPDiagnosticsToMarkers.java | 104 +++++++++--------- 2 files changed, 98 insertions(+), 64 deletions(-) diff --git a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/diagnostics/DiagnosticsTest.java b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/diagnostics/DiagnosticsTest.java index 6086b9a68..0d6ef5b99 100644 --- a/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/diagnostics/DiagnosticsTest.java +++ b/org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/diagnostics/DiagnosticsTest.java @@ -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; @@ -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; @@ -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]; @@ -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 @@ -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; + })); } @@ -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]; @@ -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 @@ -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); } diff --git a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/diagnostics/LSPDiagnosticsToMarkers.java b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/diagnostics/LSPDiagnosticsToMarkers.java index fdb94db8a..432a637da 100644 --- a/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/diagnostics/LSPDiagnosticsToMarkers.java +++ b/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/diagnostics/LSPDiagnosticsToMarkers.java @@ -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; @@ -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( - 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(); - final var toUpdate = new HashMap(); + 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 markerAttributes = computeMarkerAttributes(document, diagnostic, resource); - resource.createMarker(markerType, markerAttributes); - } - for (Entry entry : toUpdate.entrySet()) { - Map 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( + 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(); + final var toUpdate = new HashMap(); + + // 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 markerAttributes = computeMarkerAttributes(document, diagnostic, resource); + resource.createMarker(markerType, markerAttributes); + } + for (Entry entry : toUpdate.entrySet()) { + Map 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 targetAttributes, @NonNull IMarker marker) {