-
Notifications
You must be signed in to change notification settings - Fork 67
fix: Avoid UI freezes on Open Declaration with slow lang servers #1376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| /******************************************************************************* | ||
| * Copyright (c) 2025 Vegard IT GmbH and others. | ||
| * This program and the accompanying materials are made | ||
| * available under the terms of the Eclipse Public License 2.0 | ||
| * which is available at https://www.eclipse.org/legal/epl-2.0/ | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| * | ||
| * Contributors: | ||
| * Sebastian Thomschke (Vegard IT GmbH) - initial implementation. | ||
| *******************************************************************************/ | ||
| package org.eclipse.lsp4e.internal; | ||
|
|
||
| import java.time.Duration; | ||
| import java.util.Collections; | ||
| import java.util.Map; | ||
| import java.util.WeakHashMap; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.ConcurrentMap; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.function.Supplier; | ||
|
|
||
| import org.eclipse.jdt.annotation.Nullable; | ||
| import org.eclipse.jface.text.IDocument; | ||
| import org.eclipse.jface.text.IDocumentExtension4; | ||
|
|
||
| /** | ||
| * Generic, per-document+offset cache for asynchronous results that avoids | ||
| * starting the same work twice by sharing a single running task. | ||
| * | ||
| * <p> | ||
| * Features: | ||
| * <li>Weakly keys by {@link IDocument} to avoid memory leaks. | ||
| * <li>Per-document concurrent maps for thread-safe access from UI and | ||
| * background. | ||
| * <li>Eviction: TTL-based using {@link System#nanoTime()} and document-change | ||
| * invalidation when a stable modification stamp is available. | ||
| * <li>In-flight de-duplication: only one running task per document+offset. | ||
| * <li>Stale-result protection: if the document changes while a value is being | ||
| * computed, the result is delivered to callers but is not cached. | ||
| */ | ||
| public final class DocumentOffsetAsyncCache<V> { | ||
|
|
||
| private record Entry<V>(V value, long createdNanos, long docModStamp) { | ||
| boolean stale(final long ttlNanos, final long currentDocStamp) { | ||
| return System.nanoTime() - createdNanos > ttlNanos // | ||
| // Invalidate when we can confidently detect a document change | ||
| || (docModStamp != IDocumentExtension4.UNKNOWN_MODIFICATION_STAMP | ||
| && currentDocStamp != IDocumentExtension4.UNKNOWN_MODIFICATION_STAMP | ||
| && docModStamp != currentDocStamp); | ||
| } | ||
| } | ||
|
|
||
| private final Map<IDocument, ConcurrentMap<Integer, Entry<V>>> cache = Collections | ||
| .synchronizedMap(new WeakHashMap<>()); | ||
| private final Map<IDocument, ConcurrentMap<Integer, CompletableFuture<V>>> inFlight = Collections | ||
| .synchronizedMap(new WeakHashMap<>()); | ||
|
|
||
| private final long ttlNanos; | ||
|
|
||
| public DocumentOffsetAsyncCache(final Duration ttl) { | ||
| this.ttlNanos = TimeUnit.MILLISECONDS.toNanos(ttl.toMillis()); | ||
| } | ||
|
|
||
| /** | ||
| * Returns cached value if present and valid; otherwise returns the single | ||
| * running task or starts one via {@code supplier}. A value is valid if it has | ||
| * not expired by TTL and (when stamps are available) matches the current | ||
| * document stamp. Results computed for an older stamp are not cached. | ||
| */ | ||
| public CompletableFuture<V> computeIfAbsent(final IDocument doc, final int offset, | ||
| final Supplier<CompletableFuture<V>> supplier) { | ||
| // Fast path: return a completed future if a fresh value is already cached | ||
| final @Nullable V cachedNow = getNow(doc, offset); | ||
| if (cachedNow != null) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in this PR, ifs with only one statement do not have braces around the statement. I think LSP4E has the convention so far to always have braces, doesn't it? Can we have them here as well?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK we do not use braces for single return statement. At least there are plenty of cases like this. |
||
| return CompletableFuture.completedFuture(cachedNow); | ||
|
|
||
| final ConcurrentMap<Integer, CompletableFuture<V>> byOffset = inFlight.computeIfAbsent(doc, | ||
| d -> new ConcurrentHashMap<>()); | ||
| return byOffset.computeIfAbsent(offset, k -> { | ||
| final long startStamp = DocumentUtil.getDocumentModificationStamp(doc); | ||
| final CompletableFuture<V> cf = supplier.get(); | ||
| cf.whenComplete((v, t) -> { | ||
| // Always clean up the in-flight entry by key. Only one future exists | ||
| // per offset due to computeIfAbsent, so this is safe and avoids capturing | ||
| // a specific future instance. | ||
| byOffset.remove(offset); | ||
| if (t == null && v != null) { | ||
| final long nowStamp = DocumentUtil.getDocumentModificationStamp(doc); | ||
| if (startStamp == IDocumentExtension4.UNKNOWN_MODIFICATION_STAMP | ||
| || nowStamp == IDocumentExtension4.UNKNOWN_MODIFICATION_STAMP || nowStamp == startStamp) { | ||
| put(doc, offset, v); | ||
| } | ||
| } | ||
| }); | ||
| return cf; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * @return the cached value if present and valid; removes and returns null if | ||
| * TTL expired or the document stamp changed. | ||
| */ | ||
| public @Nullable V getNow(final IDocument doc, final int offset) { | ||
| final ConcurrentMap<Integer, Entry<V>> byOffset = cache.get(doc); | ||
|
rubenporras marked this conversation as resolved.
|
||
| if (byOffset == null) | ||
| return null; | ||
|
|
||
| final Entry<V> e = byOffset.get(offset); | ||
| if (e == null) | ||
| return null; | ||
|
|
||
| final long nowStamp = DocumentUtil.getDocumentModificationStamp(doc); | ||
| if (e.stale(ttlNanos, nowStamp)) { | ||
| byOffset.remove(offset, e); | ||
| return null; | ||
| } | ||
| return e.value; | ||
| } | ||
|
|
||
| public void invalidate(final IDocument doc) { | ||
| cache.remove(doc); // synchronizedMap handles its own locking | ||
| final var map = inFlight.remove(doc); // remove returns the per-doc map, if any | ||
| if (map != null) { | ||
| map.values().forEach(f -> f.cancel(true)); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Stores a value tagged with the current document modification stamp. | ||
| */ | ||
| public void put(final IDocument doc, final int offset, final V value) { | ||
| cache.compute(doc, (d, byOffset) -> { | ||
| final ConcurrentMap<Integer, Entry<V>> map = byOffset != null ? byOffset : new ConcurrentHashMap<>(); | ||
| final long stamp = DocumentUtil.getDocumentModificationStamp(doc); | ||
| map.put(offset, new Entry<>(value, System.nanoTime(), stamp)); | ||
| return map; | ||
| }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| /******************************************************************************* | ||
| * Copyright (c) 2025 Vegard IT GmbH and others. | ||
| * This program and the accompanying materials are made | ||
| * available under the terms of the Eclipse Public License 2.0 | ||
| * which is available at https://www.eclipse.org/legal/epl-2.0/ | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| * | ||
| * Contributors: | ||
| * Sebastian Thomschke (Vegard IT GmbH) - initial implementation. | ||
| *******************************************************************************/ | ||
| package org.eclipse.lsp4e.operations.declaration; | ||
|
|
||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.CompletionException; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import org.eclipse.jdt.annotation.Nullable; | ||
| import org.eclipse.jface.text.IDocument; | ||
| import org.eclipse.jface.text.IDocumentExtension4; | ||
| import org.eclipse.jface.text.IRegion; | ||
| import org.eclipse.jface.text.ITextViewer; | ||
| import org.eclipse.jface.text.hyperlink.IHyperlink; | ||
| import org.eclipse.lsp4e.LanguageServerPlugin; | ||
| import org.eclipse.lsp4e.internal.DocumentUtil; | ||
|
|
||
| /** | ||
| * An implementation of {@link IHyperlink} which asynchronously opens the link | ||
| * once the language server has responded. Opening is dismissed if the editor | ||
| * was closed in the meantime, the document was modified, or the response took | ||
| * longer than a given timeout. | ||
| */ | ||
| final class DeferredOpenDeclarationHyperlink implements IHyperlink { | ||
|
sebthom marked this conversation as resolved.
|
||
|
|
||
| private static final long DEFERRED_OPEN_TIMEOUT_NANOS = TimeUnit.SECONDS.toNanos(5); | ||
|
|
||
| private final ITextViewer viewer; | ||
| private final IDocument document; | ||
| private final long documentInitialModificationStamp; | ||
| private final IRegion region; | ||
| private final CompletableFuture<@Nullable IHyperlink> future; | ||
| private final long createdNanos = System.nanoTime(); | ||
|
|
||
| DeferredOpenDeclarationHyperlink(final ITextViewer viewer, final IDocument document, final IRegion region, | ||
| final CompletableFuture<@Nullable IHyperlink> future) { | ||
| this.viewer = viewer; | ||
| this.document = document; | ||
| this.region = region; | ||
| this.future = future; | ||
| this.documentInitialModificationStamp = DocumentUtil.getDocumentModificationStamp(document); | ||
| } | ||
|
|
||
| @Override | ||
| public IRegion getHyperlinkRegion() { | ||
| return region; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable String getTypeLabel() { | ||
| final var link = getResolvedLink(); | ||
| return link != null ? link.getTypeLabel() : null; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable String getHyperlinkText() { | ||
| final var link = getResolvedLink(); | ||
| return link != null ? link.getHyperlinkText() : null; | ||
| } | ||
|
|
||
| @Override | ||
| public void open() { | ||
| future.whenComplete((link, ex) -> { | ||
| if (ex != null) { | ||
| LanguageServerPlugin.logError(ex.getLocalizedMessage(), ex); | ||
| return; | ||
| } | ||
| final var widget = viewer.getTextWidget(); | ||
| if (widget == null) | ||
| return; | ||
| if (link == null) { | ||
| LanguageServerPlugin.logWarning("No hyperlink target resolved for Open Declaration"); //$NON-NLS-1$ | ||
| return; | ||
| } | ||
| widget.getDisplay().asyncExec(() -> { | ||
| if (isStale()) | ||
| return; | ||
| link.open(); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| private @Nullable IHyperlink getResolvedLink() { | ||
| try { | ||
| return future.getNow(null); | ||
| } catch (CompletionException ex) { | ||
| LanguageServerPlugin.logError(ex.getLocalizedMessage(), ex); | ||
| return null; | ||
|
sebthom marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
||
| private boolean isStale() { | ||
| // LS response came too late? | ||
| if (System.nanoTime() - createdNanos > DEFERRED_OPEN_TIMEOUT_NANOS) | ||
| return true; | ||
|
|
||
| // Editor was closed? | ||
| final var widget = viewer.getTextWidget(); | ||
| if (widget == null || widget.isDisposed()) | ||
| return true; | ||
|
|
||
| // Document was modified? | ||
| if (documentInitialModificationStamp != IDocumentExtension4.UNKNOWN_MODIFICATION_STAMP | ||
| && DocumentUtil.getDocumentModificationStamp(document) != documentInitialModificationStamp) | ||
| return true; | ||
|
|
||
| return false; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,138 @@ | ||
| /******************************************************************************* | ||
| * Copyright (c) 2025 Vegard IT GmbH and others. | ||
| * This program and the accompanying materials are made | ||
| * available under the terms of the Eclipse Public License 2.0 | ||
| * which is available at https://www.eclipse.org/legal/epl-2.0/ | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| * | ||
| * Contributors: | ||
| * Sebastian Thomschke (Vegard IT GmbH) - initial implementation. | ||
| ******************************************************************************/ | ||
| package org.eclipse.lsp4e.operations.declaration; | ||
|
|
||
| import java.util.List; | ||
| import java.util.concurrent.CompletableFuture; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| import org.eclipse.jdt.annotation.Nullable; | ||
| import org.eclipse.jface.text.IDocument; | ||
| import org.eclipse.jface.text.IDocumentExtension4; | ||
| import org.eclipse.jface.text.IRegion; | ||
| import org.eclipse.jface.text.ITextViewer; | ||
| import org.eclipse.jface.text.hyperlink.IHyperlink; | ||
| import org.eclipse.jface.viewers.LabelProvider; | ||
| import org.eclipse.jface.window.Window; | ||
| import org.eclipse.lsp4e.LanguageServerPlugin; | ||
| import org.eclipse.lsp4e.internal.DocumentUtil; | ||
| import org.eclipse.lsp4e.ui.Messages; | ||
| import org.eclipse.swt.widgets.Shell; | ||
| import org.eclipse.ui.dialogs.ElementListSelectionDialog; | ||
|
|
||
| /** | ||
| * An implementation of {@link IHyperlink} which asynchronously opens a chooser | ||
| * of links once the language server has responded. Opening is dismissed if the | ||
| * editor was closed in the meantime, the document was modified, or the response | ||
| * took longer than a given timeout. | ||
| */ | ||
| final class DeferredOpenMultiDeclarationHyperlink implements IHyperlink { | ||
|
|
||
| private static final long DEFERRED_OPEN_TIMEOUT_NANOS = TimeUnit.SECONDS.toNanos(5); | ||
|
|
||
| private final ITextViewer viewer; | ||
| private final IDocument document; | ||
| private final long documentInitialModificationStamp; | ||
| private final IRegion region; | ||
| private final CompletableFuture<? extends List<? extends IHyperlink>> future; | ||
| private final long createdNanos = System.nanoTime(); | ||
|
|
||
| DeferredOpenMultiDeclarationHyperlink(final ITextViewer viewer, final IDocument document, final IRegion region, | ||
| final CompletableFuture<? extends List<? extends IHyperlink>> future) { | ||
| this.viewer = viewer; | ||
| this.document = document; | ||
| this.region = region; | ||
| this.future = future; | ||
| this.documentInitialModificationStamp = DocumentUtil.getDocumentModificationStamp(document); | ||
| } | ||
|
|
||
| @Override | ||
| public IRegion getHyperlinkRegion() { | ||
| return region; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable String getTypeLabel() { | ||
| return "Open Declaration (resolving...)"; //$NON-NLS-1$ | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable String getHyperlinkText() { | ||
| return "Open Declaration (resolving...)"; //$NON-NLS-1$ | ||
| } | ||
|
|
||
| @Override | ||
| public void open() { | ||
| future.whenComplete((links, ex) -> { | ||
| if (ex != null) { | ||
| LanguageServerPlugin.logError(ex.getLocalizedMessage(), ex); | ||
| return; | ||
| } | ||
| final var widget = viewer.getTextWidget(); | ||
| if (widget == null) | ||
| return; | ||
| if (links.isEmpty()) { | ||
| LanguageServerPlugin.logWarning("No hyperlink targets resolved for Open Declaration"); //$NON-NLS-1$ | ||
| return; | ||
| } | ||
| widget.getDisplay().asyncExec(() -> { | ||
| if (isStale()) | ||
| return; | ||
|
|
||
| if (links.size() == 1) { | ||
| links.get(0).open(); | ||
| return; | ||
| } | ||
|
|
||
| final Shell shell = widget.getShell(); | ||
| final var dialog = new ElementListSelectionDialog(shell, new LabelProvider() { | ||
| @Override | ||
| public String getText(final @Nullable Object element) { | ||
| if (element instanceof final IHyperlink link) { | ||
| final String text = link.getHyperlinkText(); | ||
| return text != null ? text : link.getTypeLabel(); | ||
| } | ||
| return element == null ? "" : element.toString(); //$NON-NLS-1$ | ||
| } | ||
| }); | ||
| dialog.setTitle(Messages.declarationHyperlinkLabel); | ||
| dialog.setMessage("Select a target:"); //$NON-NLS-1$ | ||
| dialog.setElements(links.toArray()); | ||
| dialog.setMultipleSelection(false); | ||
| if (dialog.open() == Window.OK) { | ||
| Object result = dialog.getFirstResult(); | ||
| if (result instanceof IHyperlink link) { | ||
| link.open(); | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| private boolean isStale() { | ||
| // LS response came too late? | ||
| if (System.nanoTime() - createdNanos > DEFERRED_OPEN_TIMEOUT_NANOS) | ||
| return true; | ||
|
|
||
| // Editor was closed? | ||
| final var widget = viewer.getTextWidget(); | ||
| if (widget == null || widget.isDisposed()) | ||
| return true; | ||
|
|
||
| // Document was modified? | ||
| if (documentInitialModificationStamp != IDocumentExtension4.UNKNOWN_MODIFICATION_STAMP | ||
| && DocumentUtil.getDocumentModificationStamp(document) != documentInitialModificationStamp) | ||
| return true; | ||
|
|
||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we convert the time to nanos? Can we is millis not a good enough resolution, specially given that the input is millis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duration calculations based on System.currentTimeMillis() is now considered bad practices as it is not time shift/leap second aware and can result in wrong calculations. Therefore one is supposted to use System.nanoTime() which returns nano seconds for measuring elapsed time.