From 185b666253f97906138f4d201450a3823a7e7711 Mon Sep 17 00:00:00 2001 From: Kris De Volder Date: Mon, 17 Apr 2017 12:28:21 -0700 Subject: [PATCH] Fix race-condition: reconciler versus ast cache --- .../util/LanguageServerTestListener.java | 22 +++++ .../util/SimpleLanguageServer.java | 28 +++++- .../util/SimpleTextDocumentService.java | 28 +++--- .../util/TextDocumentContentChange.java | 12 +-- .../vscode/commons/util/text/IDocument.java | 1 + .../commons/util/text/TextDocument.java | 25 ++++-- .../vscode/java/properties/parser/Parser.java | 6 +- .../testharness/LanguageServerHarness.java | 28 ++++++ .../testharness/SynchronizationPoint.java | 41 +++++++++ .../ide/vscode/concourse/ConcourseModel.java | 12 +-- .../concourse/util/StaleFallbackCache.java | 87 ++++++++++++++----- .../vscode/concourse/ConcourseEditorTest.java | 27 ++++++ 12 files changed, 251 insertions(+), 66 deletions(-) create mode 100644 headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/LanguageServerTestListener.java create mode 100644 headless-services/commons/language-server-test-harness/src/main/java/org/springframework/ide/vscode/languageserver/testharness/SynchronizationPoint.java diff --git a/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/LanguageServerTestListener.java b/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/LanguageServerTestListener.java new file mode 100644 index 000000000..1350e6a0d --- /dev/null +++ b/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/LanguageServerTestListener.java @@ -0,0 +1,22 @@ +/******************************************************************************* + * Copyright (c) 2017 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 + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Pivotal, Inc. - initial API and implementation + *******************************************************************************/ +package org.springframework.ide.vscode.commons.languageserver.util; + +/** + * A listener used only for testing purposes. It can be attached to a {@link SimpleLanguageServer} + * to allow tests to receive callbacks for certain 'interesting' points in the language server's + * processing. + * + * @author Kris De Volder + */ +public interface LanguageServerTestListener { + void reconcileStarted(String uri, int version); +} diff --git a/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/SimpleLanguageServer.java b/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/SimpleLanguageServer.java index eaa4dc8df..a96222064 100644 --- a/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/SimpleLanguageServer.java +++ b/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/SimpleLanguageServer.java @@ -45,6 +45,7 @@ import org.springframework.ide.vscode.commons.languageserver.reconcile.IProblemC import org.springframework.ide.vscode.commons.languageserver.reconcile.IReconcileEngine; import org.springframework.ide.vscode.commons.languageserver.reconcile.ProblemSeverity; import org.springframework.ide.vscode.commons.languageserver.reconcile.ReconcileProblem; +import org.springframework.ide.vscode.commons.util.Assert; import org.springframework.ide.vscode.commons.util.BadLocationException; import org.springframework.ide.vscode.commons.util.CollectionUtil; import org.springframework.ide.vscode.commons.util.Log; @@ -87,6 +88,8 @@ public abstract class SimpleLanguageServer implements LanguageServer, LanguageCl private QuickfixRegistry quickfixRegistry; + private LanguageServerTestListener testListener; + @Override public void connect(LanguageClient _client) { this.client = (STS4LanguageClient) _client; @@ -225,14 +228,23 @@ public abstract class SimpleLanguageServer implements LanguageServer, LanguageCl */ protected void validateWith(TextDocumentIdentifier docId, IReconcileEngine engine) { CompletableFuture reconcileSession = this.busyReconcile = new CompletableFuture(); - Log.debug("Reconciling BUSY"); +// Log.debug("Reconciling BUSY"); SimpleTextDocumentService documents = getTextDocumentService(); + int requestedVersion = documents.getDocument(docId.getUri()).getVersion(); + // Avoid running in the same thread as lsp4j as it can result // in long "hangs" for slow reconcile providers Mono.fromRunnable(() -> { TextDocument doc = documents.getDocument(docId.getUri()).copy(); + if (requestedVersion!=doc.getVersion()) { + //Do not bother reconciling if document contents is already stale. + return; + } + if (testListener!=null) { + testListener.reconcileStarted(docId.getUri(), doc.getVersion()); + } IProblemCollector problems = new IProblemCollector() { private List diagnostics = new ArrayList<>(); @@ -278,12 +290,15 @@ public abstract class SimpleLanguageServer implements LanguageServer, LanguageCl // Thread.sleep(2000); // } catch (InterruptedException e) { // } - Log.debug("Reconciling: "+doc); engine.reconcile(doc, problems); }) - .doOnTerminate((ignore1, ignore2) -> { + .otherwise(error -> { + Log.log(error); + return Mono.empty(); + }) + .doFinally(ignore -> { reconcileSession.complete(null); - Log.debug("Reconciler DONE : "+this.busyReconcile.isDone()); +// Log.debug("Reconciler DONE : "+this.busyReconcile.isDone()); }) .subscribeOn(RECONCILER_SCHEDULER) .subscribe(); @@ -327,5 +342,10 @@ public abstract class SimpleLanguageServer implements LanguageServer, LanguageCl return quickfixes.handle(params); } + public void setTestListener(LanguageServerTestListener languageServerTestListener) { + Assert.isLegal(this.testListener==null); + testListener = languageServerTestListener; + } + } diff --git a/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/SimpleTextDocumentService.java b/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/SimpleTextDocumentService.java index ed1df9f94..e30301ab7 100644 --- a/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/SimpleTextDocumentService.java +++ b/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/SimpleTextDocumentService.java @@ -129,14 +129,12 @@ public class SimpleTextDocumentService implements TextDocumentService { try { VersionedTextDocumentIdentifier docId = params.getTextDocument(); String url = docId.getUri(); - Log.debug("didChange: "+url); +// Log.debug("didChange: "+url); if (url!=null) { TextDocument doc = getDocument(url); - for (TextDocumentContentChangeEvent change : params.getContentChanges()) { - doc.apply(change); - didChangeContent(doc, change); - } - doc.setVersion(docId.getVersion()); + List changes = params.getContentChanges(); + doc.apply(params); + didChangeContent(doc, changes); } } catch (BadLocationException e) { Log.log(e); @@ -151,8 +149,7 @@ public class SimpleTextDocumentService implements TextDocumentService { int version = docId.getVersion(); if (url!=null) { String text = params.getTextDocument().getText(); - TextDocument doc = createDocument(url, languageId, version).getDocument(); - doc.setText(text); + TextDocument doc = createDocument(url, languageId, version, text).getDocument(); TextDocumentContentChangeEvent change = new TextDocumentContentChangeEvent() { @Override public Range getRange() { @@ -169,7 +166,7 @@ public class SimpleTextDocumentService implements TextDocumentService { return text; } }; - TextDocumentContentChange evt = new TextDocumentContentChange(doc, change); + TextDocumentContentChange evt = new TextDocumentContentChange(doc, ImmutableList.of(change)); documentChangeListeners.fire(evt); } } @@ -183,8 +180,8 @@ public class SimpleTextDocumentService implements TextDocumentService { } } - void didChangeContent(TextDocument doc, TextDocumentContentChangeEvent change) { - documentChangeListeners.fire(new TextDocumentContentChange(doc, change)); + void didChangeContent(TextDocument doc, List changes) { + documentChangeListeners.fire(new TextDocumentContentChange(doc, changes)); } public void onDidChangeContent(Consumer l) { @@ -195,16 +192,16 @@ public class SimpleTextDocumentService implements TextDocumentService { TrackedDocument doc = documents.get(url); if (doc==null) { Log.warn("Trying to get document ["+url+"] but it did not exists. Creating it with language-id 'plaintext'"); - doc = createDocument(url, LanguageIds.PLAINTEXT, 0); + doc = createDocument(url, LanguageIds.PLAINTEXT, 0, ""); } return doc.getDocument(); } - private synchronized TrackedDocument createDocument(String url, String languageId, int version) { + private synchronized TrackedDocument createDocument(String url, String languageId, int version, String text) { if (documents.get(url)!=null) { Log.warn("Creating document ["+url+"] but it already exists. Existing document discarded!"); } - TrackedDocument doc = new TrackedDocument(new TextDocument(url, languageId, version)); + TrackedDocument doc = new TrackedDocument(new TextDocument(url, languageId, version, text)); documents.put(url, doc); return doc; } @@ -273,9 +270,7 @@ public class SimpleTextDocumentService implements TextDocumentService { return CompletableFuture.completedFuture(ImmutableList.of()); } return Mono.fromCallable(() -> { - Log.debug("documentSymbol request waiting for reconcile: "+params.getTextDocument()); server.waitForReconcile(); - Log.info("documentSymbol request proceeding: "+params.getTextDocument()); return documentSymbolHandler.handle(params); }) .toFuture() @@ -338,7 +333,6 @@ public class SimpleTextDocumentService implements TextDocumentService { params.setUri(docId.getUri()); params.setDiagnostics(diagnostics); client.publishDiagnostics(params); - //Log.info("publishDiagnostics: "+params); } } diff --git a/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/TextDocumentContentChange.java b/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/TextDocumentContentChange.java index 70cdce149..4c45a9eec 100644 --- a/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/TextDocumentContentChange.java +++ b/headless-services/commons/commons-language-server/src/main/java/org/springframework/ide/vscode/commons/languageserver/util/TextDocumentContentChange.java @@ -11,25 +11,27 @@ package org.springframework.ide.vscode.commons.languageserver.util; +import java.util.List; + import org.eclipse.lsp4j.TextDocumentContentChangeEvent; import org.springframework.ide.vscode.commons.util.text.TextDocument; public class TextDocumentContentChange { private final TextDocument document; - private final TextDocumentContentChangeEvent change; + private final List changes; - public TextDocumentContentChange(TextDocument doc, TextDocumentContentChangeEvent change) { + public TextDocumentContentChange(TextDocument doc, List changes) { this.document = doc; - this.change = change; + this.changes = changes; } public TextDocument getDocument() { return document; } - public TextDocumentContentChangeEvent getChange() { - return change; + public List getChanges() { + return changes; } } diff --git a/headless-services/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/IDocument.java b/headless-services/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/IDocument.java index ed2385e53..d16658915 100644 --- a/headless-services/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/IDocument.java +++ b/headless-services/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/IDocument.java @@ -29,5 +29,6 @@ public interface IDocument { void replace(int start, int len, String text) throws BadLocationException; String textBetween(int start, int end) throws BadLocationException; String getLanguageId(); + int getVersion(); } diff --git a/headless-services/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/TextDocument.java b/headless-services/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/TextDocument.java index c852184d1..d484f0170 100644 --- a/headless-services/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/TextDocument.java +++ b/headless-services/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/TextDocument.java @@ -11,13 +11,16 @@ package org.springframework.ide.vscode.commons.util.text; +import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.eclipse.lsp4j.DidChangeTextDocumentParams; import org.eclipse.lsp4j.Position; import org.eclipse.lsp4j.Range; import org.eclipse.lsp4j.TextDocumentContentChangeEvent; import org.eclipse.lsp4j.TextDocumentIdentifier; +import org.springframework.ide.vscode.commons.util.Assert; import org.springframework.ide.vscode.commons.util.BadLocationException; import org.springframework.ide.vscode.commons.util.text.linetracker.DefaultLineTracker; import org.springframework.ide.vscode.commons.util.text.linetracker.ILineTracker; @@ -35,7 +38,7 @@ public class TextDocument implements IDocument { private int version; public TextDocument(String uri, String languageId) { - this(uri, languageId, 0); + this(uri, languageId, 0, ""); } private TextDocument(TextDocument other) { @@ -46,10 +49,11 @@ public class TextDocument implements IDocument { this.version = other.version; } - public TextDocument(String uri, String languageId, int version) { + public TextDocument(String uri, String languageId, int version, String text) { this.uri = uri; this.languageId = languageId; this.version = version; + setText(text); } @Override @@ -71,7 +75,7 @@ public class TextDocument implements IDocument { this.lineTracker.set(text); } - public void apply(TextDocumentContentChangeEvent change) throws BadLocationException { + private void apply(TextDocumentContentChangeEvent change) throws BadLocationException { Range rng = change.getRange(); if (rng==null) { //full sync mode @@ -83,6 +87,15 @@ public class TextDocument implements IDocument { } } + public synchronized void apply(DidChangeTextDocumentParams params) throws BadLocationException { + int newVersion = params.getTextDocument().getVersion(); + Assert.isLegal(version blocker = new CompletableFuture<>(); + server.setTestListener(new LanguageServerTestListener() { + @Override + public void reconcileStarted(String uri, int version) { + try { + blocker.get(); + } catch (Exception e) { + throw ExceptionUtil.unchecked(e); + } + } + }); + return new SynchronizationPoint() { + @Override public void unblock() { + blocker.complete(null); + } + @Override public Future reached() { + return blocker; + } + }; + } } diff --git a/headless-services/commons/language-server-test-harness/src/main/java/org/springframework/ide/vscode/languageserver/testharness/SynchronizationPoint.java b/headless-services/commons/language-server-test-harness/src/main/java/org/springframework/ide/vscode/languageserver/testharness/SynchronizationPoint.java new file mode 100644 index 000000000..086fcea7f --- /dev/null +++ b/headless-services/commons/language-server-test-harness/src/main/java/org/springframework/ide/vscode/languageserver/testharness/SynchronizationPoint.java @@ -0,0 +1,41 @@ +/******************************************************************************* + * Copyright (c) 2017 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 + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Pivotal, Inc. - initial API and implementation + *******************************************************************************/ +package org.springframework.ide.vscode.languageserver.testharness; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Future; + +/** + * Represents a synchronization point in execution of some code. + *

+ * This is used by testing code to be able to block a thread in + * a manner controlled by the test flow. + *

+ * This is a useful tool to allow creating test for + * race conditions. + * + * @author Kris De Volder + */ +public interface SynchronizationPoint { + + /** + * Returns a future that resolves when the synchronization + * point is reached. + */ + Future reached(); + + /** + * Unblocks the thread(s) that have reached the synchronization + * point. + */ + void unblock(); + +} diff --git a/headless-services/concourse-language-server/src/main/java/org/springframework/ide/vscode/concourse/ConcourseModel.java b/headless-services/concourse-language-server/src/main/java/org/springframework/ide/vscode/concourse/ConcourseModel.java index 357f6d034..7d9a220ea 100644 --- a/headless-services/concourse-language-server/src/main/java/org/springframework/ide/vscode/concourse/ConcourseModel.java +++ b/headless-services/concourse-language-server/src/main/java/org/springframework/ide/vscode/concourse/ConcourseModel.java @@ -121,19 +121,9 @@ public class ConcourseModel { private final ASTTypeCache astTypes = new ASTTypeCache(); - public ConcourseModel(SimpleTextDocumentService documents) { Yaml yaml = new Yaml(); this.parser = new YamlParser(yaml); - documents.onDidChangeContent(this::documentChanged); - } - - private void documentChanged(TextDocumentContentChange changeEvent) { - String uri = changeEvent.getDocument().getUri(); - if (uri!=null) { - Log.debug("Clear AST cache: "+uri); - asts.invalidate(uri); - } } /** @@ -253,7 +243,7 @@ public class ConcourseModel { return (IDocument doc) -> { String uri = doc.getUri(); if (uri!=null) { - return asts.get(uri, allowStaleAsts, () -> { + return asts.get(uri, doc.getVersion(), allowStaleAsts, () -> { return parser.getAST(doc); }); } diff --git a/headless-services/concourse-language-server/src/main/java/org/springframework/ide/vscode/concourse/util/StaleFallbackCache.java b/headless-services/concourse-language-server/src/main/java/org/springframework/ide/vscode/concourse/util/StaleFallbackCache.java index 1f76a6135..069c636ee 100644 --- a/headless-services/concourse-language-server/src/main/java/org/springframework/ide/vscode/concourse/util/StaleFallbackCache.java +++ b/headless-services/concourse-language-server/src/main/java/org/springframework/ide/vscode/concourse/util/StaleFallbackCache.java @@ -28,40 +28,87 @@ import com.google.common.cache.CacheBuilder; */ public class StaleFallbackCache{ - Map staleEntries = new HashMap<>(); - Cache> validEntries = CacheBuilder.newBuilder().build(); + private static class Versioned { + int version; + T it; + public Versioned(int version, T it) { + super(); + this.version = version; + this.it = it; + } + @Override + public int hashCode() { + final int prime = 31; + int result = 1; + result = prime * result + ((it == null) ? 0 : it.hashCode()); + result = prime * result + version; + return result; + } + @Override + public boolean equals(Object obj) { + if (this == obj) + return true; + if (obj == null) + return false; + if (getClass() != obj.getClass()) + return false; + Versioned other = (Versioned) obj; + if (it == null) { + if (other.it != null) + return false; + } else if (!it.equals(other.it)) + return false; + if (version != other.version) + return false; + return true; + } + @Override + public String toString() { + return "Versioned [version=" + version + ", it=" + it + "]"; + } + } - public synchronized V get(K key, boolean allowStaleEntries, Callable valueLoader) throws Exception { - CompletableFuture valid = validEntries.get(key, () -> load(valueLoader)); + Map staleEntries = new HashMap<>(); + Cache>> latestEntries = CacheBuilder.newBuilder().build(); + + public synchronized V get(K key, int version, boolean allowStaleEntries, Callable valueLoader) throws Exception { + Versioned> latest = latestEntries.get(key, () -> new Versioned<>(version, load(valueLoader))); + if (latest.version!=version) { + latestEntries.invalidate(key); + keepStaleBackup(key, latest); + latest = latestEntries.get(key, () -> new Versioned<>(version, load(valueLoader))); + } if (!allowStaleEntries) { - return future_get(valid); + return future_get(version, latest); } else { - if (valid.isCompletedExceptionally()) { + if (latest.it.isCompletedExceptionally()) { V staleValue = staleEntries.get(key); if (staleValue!=null) { return staleValue; } } - return future_get(valid); + return future_get(latest.it); } } - public synchronized void invalidate(K key) { - CompletableFuture staleEntry = validEntries.getIfPresent(key); - if (staleEntry!=null) { - validEntries.invalidate(key); - try { - staleEntries.put(key, future_get(staleEntry)); - } catch (Exception e) { - //ignore. Don't overwrite stale entry if current entry represents an error. - // We only keep 'good quality' stale entries not failed attempts to compute a value. - // as it is kind of the point to fall back on a 'good' old entry when the current - // entry is unavailable because of a problem (e.g. problems parsing the AST). - } + /** + * Called when a stale entry is found in the 'latest' map. This method is + * responsible for determining if the entry should be kept as a staleBackup, + * and store it. + */ + private void keepStaleBackup(K key, Versioned> latest) { + try { + staleEntries.put(key, latest.it.get()); + } catch (InterruptedException | ExecutionException e) { + //ignore: This means its a 'bad' entry and so we don't want to keep it + // as a 'stale backup'. } } - + private V future_get(int wantedVersion, Versioned> versioned) throws Exception { + Assert.isLegal(wantedVersion==versioned.version); + return future_get(versioned.it); + } private V future_get(CompletableFuture f) throws Exception { try { diff --git a/headless-services/concourse-language-server/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java b/headless-services/concourse-language-server/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java index dc1775031..e1fd5553e 100644 --- a/headless-services/concourse-language-server/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java +++ b/headless-services/concourse-language-server/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java @@ -27,6 +27,7 @@ import org.springframework.ide.vscode.commons.util.IOUtil; import org.springframework.ide.vscode.languageserver.testharness.CodeAction; import org.springframework.ide.vscode.languageserver.testharness.Editor; import org.springframework.ide.vscode.languageserver.testharness.LanguageServerHarness; +import org.springframework.ide.vscode.languageserver.testharness.SynchronizationPoint; public class ConcourseEditorTest { @@ -2985,6 +2986,32 @@ public class ConcourseEditorTest { ); } + @Test public void reconcilerRaceCondition() throws Exception { + SynchronizationPoint reconcilerThreadStart = harness.reconcilerThreadStart(); + Editor editor = harness.newEditor("garbage"); + + reconcilerThreadStart.reached(); // Blocks until the reconciler thread is reached. + try { + String editorContents = editor.getRawText(); + for (int i = 0; i < 4; i++) { + editorContents = "\n" +editorContents; + editor.setText(editorContents); + } + } finally { + reconcilerThreadStart.unblock(); + } + + editor.assertRawText( + "\n" + + "\n" + + "\n" + + "\n" + + "garbage" + ); + editor.assertProblems("garbage|Expecting a 'Map'"); + } + + ////////////////////////////////////////////////////////////////////////////// private void assertContextualCompletions(String conText, String textBefore, String... textAfter) throws Exception {