Skip to content

fix: prevent repeated BadLocationException logging #1443

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.

Sign up for GitHub

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

Closed
sebthom wants to merge 1 commit into from
Closed

fix: prevent repeated BadLocationException logging #1443

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion ...clipse.lsp4e/src/org/eclipse/lsp4e/operations/highlight/HighlightReconcilingStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,13 +212,30 @@ private void collectHighlights(int caretOffset, @Nullable IProgressMonitor monit
lastCacheKeyOffset = cacheKeyOffset;
}

final int docLength = document.getLength();
if (caretOffset < 0 || caretOffset > docLength) {
// Stale caret offset after document changes, skip this highlight run.
// Logging is limited to debug/trace to avoid noisy error log entries.
if (LanguageServerPlugin.DEBUG || LanguageServerPlugin.isLogTraceEnabled()) {
LanguageServerPlugin.logWarning(
"Ignoring documentHighlight for stale caret offset " + caretOffset + " (document length " //$NON-NLS-1$ //$NON-NLS-2$
+ docLength + ')');
}
return;
}

Comment on lines +215 to +226
Copy link
Copy Markdown
Contributor

@FlorianKroiss FlorianKroiss Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lightweight version of what @rubenporras probably had in mind with checking LanguageServerWrapper.getTextDocumentVersion.
But do we actually gain something from such a check, besides a different error message being logged while debugging?
With or without this check, we are still in a race condition. So I would lean towards not performing this check at all.

Copy link
Copy Markdown
Contributor

@rubenporras rubenporras Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought my proposal was very simple, and will avoid the exceptions, @sebthom: do you think my proposal would not work? If it works, I would prefer that to this

Copy link
Copy Markdown
Contributor

@rubenporras rubenporras Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal is https://github.com/eclipse-lsp4e/lsp4e/pull/1444/files

Position position;
try {
// Send the original caret offset to the LS to preserve behavior
// expected by tests and servers that distinguish positions within a word.
position = LSPEclipseUtils.toPosition(caretOffset, document);
} catch (BadLocationException e) {
LanguageServerPlugin.logError(e);
// Document may have changed again between the length check above and this call.
// Treat this as a transient, non-fatal condition and only log when debugging.
if (LanguageServerPlugin.DEBUG || LanguageServerPlugin.isLogTraceEnabled()) {
LanguageServerPlugin.logWarning(
"BadLocationException while computing documentHighlight position for offset " + caretOffset, e); //$NON-NLS-1$
}
return;
}
URI uri = LSPEclipseUtils.toUri(document);
Expand Down