`BadLocationException` when computing completions at the end of the document

[FlorianKroiss]

I also don't have reproduction steps for this one, but it seems to happen when computing completions at the end of the document while modifying it.

org.eclipse.jface.text.BadLocationException: null
	at org.eclipse.jface.text.AbstractDocument.get(AbstractDocument.java:958)
	at org.eclipse.core.internal.filebuffers.SynchronizableDocument.get(SynchronizableDocument.java:225)
	at org.eclipse.lsp4e.operations.completion.LSCompletionProposal.getPrefixCompletionStart(LSCompletionProposal.java:456)
	at org.eclipse.lsp4e.operations.completion.LSCompletionProposal.<init>(LSCompletionProposal.java:148)
	at org.eclipse.lsp4e.operations.completion.LSContentAssistProcessor.lambda$15(LSContentAssistProcessor.java:272)
	at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
	at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
	at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:575)
	at java.base/java.util.stream.AbstractPipeline.evaluateToArrayNode(AbstractPipeline.java:260)
	at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:616)
	at java.base/java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:622)
	at java.base/java.util.stream.ReferencePipeline.toList(ReferencePipeline.java:627)
	at org.eclipse.lsp4e.operations.completion.LSContentAssistProcessor.toProposals(LSContentAssistProcessor.java:278)
	at org.eclipse.lsp4e.operations.completion.LSContentAssistProcessor.lambda$2(LSContentAssistProcessor.java:152)
	at java.base/java.util.concurrent.CompletableFuture$UniAccept.tryFire(CompletableFuture.java:718)
	at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510)
	at java.base/java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2179)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.handleResponse(RemoteEndpoint.java:223)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.consume(RemoteEndpoint.java:204)
	at org.eclipse.lsp4e.LanguageServerWrapper.lambda$3(LanguageServerWrapper.java:449)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.handleMessage(StreamMessageProducer.java:185)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.listen(StreamMessageProducer.java:97)
	at org.eclipse.lsp4j.jsonrpc.json.ConcurrentMessageProcessor.run(ConcurrentMessageProcessor.java:114)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:572)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1583)

[FlorianKroiss]

The root cause for this is a race condition between a user typing in the editor and LSP4E computing completion proposals. To be more specific:

1. User types characters at the end of the file
2. LSP4E computes completion proposal at requested position
3. The user removes some characters
4. LSP4E tries to access certain ranges of the document which have become invalid due to removed characters

I think overall it is quite problematic that the computation of the completion proposals relies heavily on the current state of the IDocument which can change arbitrarily during the computation. The LSCompletionProposal also has a reference to the document which it might access at arbitrary moments to update some internal state.

From looking at the APIs of ICompletionProposal and other related interfaces I think you are only supposed to briefly access the document when it is passed in as a parameter, for example in org.eclipse.jface.text.contentassist.ICompletionProposalExtension2.validate(IDocument, int, DocumentEvent), which I believe is always called on the UI-Thread, were you are not racing against concurrent modifications by the user.

The current implementation however might access the document either from the thread which dispatches the LSP messages or from the thread which is used in AsyncCompletionProposalPopup. In either case, it is always possible that the user modified the document in the meantime, so we basically have to expect that any access to the document from these threads will produce a BadLocationException.

Since we know that these exceptions can happen here, I would argue that we should not log them, because neither the user nor LSP4E can do anything about them. Though I would prefer I we could make other changes, to be less reliant on the exact state of the document or at-least synchronize access to it with the UI-Thread?

#1479 has more or less the same root cause, only that the race condition there happens at a later stage, i.e., when sorting the proposals.

Thoughts?
@sebthom @rubenporras

[rubenporras]

Hi Florian,

My opinion is that we should implement funcionality that runs concurrently with document modifications in way that we always check the document modification timestamp/sequence before doing any document operation (as we do for example with the versioned edits) and stop processing if the document has changed.

Depending on the request, we would need to either cancel or cancel an redo the request.

I think that ignoring exceptions is in general not good, this works in some cases (like near end of document or near the end of line) we we actually get an exception, but even then it makes hard to distinguish programming errors from expected behaviour. In other cases, we present confusing information to the user because the text has moved a bit in the source or it has been replaced by something totally diferent.

[FlorianKroiss]

Hi Ruben,

Thanks for your input. I agree that we should not ignore exceptions :)

However, in this case, I don't think it's possible to use the modification stamp of the document to detect if we need to abort a completion calculation.

Let's say the user triggered a completion computation. From here, there are two interesting cases:

• Case 1: The user keeps typing more characters, which increments the modification stamp each time. In this case, Eclipse does not trigger another completion computation at each offset, but waits for the initial computation to finish and then filters the result with regard to the current cursor offset using ICompletionProposalExtension2.validate(IDocument, int, DocumentEvent). So while we are computing the completions, we can't abort when we see a changed timestamp, because the user might just continued to type, which is fine.
• Case 2: The user removes several characters, thereby moving the cursor to the left. This will retrigger the completion computation every time. If a computation is still running, it can't really cancel itself by checking the timestamp, because we might also be in case 1.
  However, if the user happens to be at the end of the document, then we will start to see BadLocationExceptions, because we are trying to access content which is no longer there (This is also what's happening in this bug). Thus, I think we can use BadLocationExceptions as an indicator that our current computation has to be cancelled and Eclipse will trigger a new one which will have the correct result.

Does that make sense?

So my idea is that we can cancel the computation as soon as we see a BadLocationException.
The only problem is that these exceptions are raised in various places, because some computations are deferred, for example, LSCompletionProposal.getRankScore() computes the score which is used for sorting. If it sees a BadLocationException it just logs it and uses a dummy score. Rethrowing the exception from there wouldn't help much either, because it is called by LSCompletionProposalComparator.compare(LSCompletionProposal, LSCompletionProposal), which by contract should not throw anything.

So that's why my initial proposal was to just ignore the exception (and add a comment why we ignore it), because the result that we are currently computing will be discarded anyway. But as you mentioned, just ignoring the exceptions is also bad practice :(

Edit:
I will try to create a PR so we can have a better look at the proposed changes

[rubenporras]

Hi Florian,

I have not noticed that Case 1 above still can reuse the computation and that therefore the modification timestamp can not be used just on its own to cancel the request.

What about registering a org.eclipse.jface.text.DocumentEvent and then cancelling only if documentAboutToBeChanged/documentChanged is called with an event that indicates that text have been remod from the document? Would that work?

[FlorianKroiss]

Hi,

I have not noticed that Case 1 above still can reuse the computation and that therefore the modification timestamp can not be used just on its own to cancel the request.

That depends if the LS that you are using is setting the isIncomplete flag on the response. If that flag is set, then the computation will be retriggered every time the cursor moves. In that case we could use the stamp to determine if we have to abort. But if the flag is not set, then we can't do that. Sorry that I did not mention that special case, but I did not want to make my previous wall of text even longer :)

If you replace these lines with the code below, you can simulate a LS without this flag. Then you will only see the "Computing!" printed once (as long you are in case 1 and don't delete so much that you end up in case 2)

lsp4e/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSContentAssistProcessor.java

Lines 150 to 151 in e2a2cdd

	boolean isIncomplete = completion != null && completion.isRight()
	&& completion.getRight().isIncomplete();

								System.out.println("Computing!"); //$NON-NLS-1$
								boolean isIncomplete = false;

What about registering a org.eclipse.jface.text.DocumentEvent and then cancelling only if documentAboutToBeChanged/documentChanged is called with an event that indicates that text have been remod from the document? Would that work?

I have to think about that.

[FlorianKroiss]

If that flag is set, then the computation will be retriggered every time the cursor moves. In that case we could use the stamp to determine if we have to abort.

I just noticed that is probably wrong. Because the user could also press DEL to remove characters on the right side of the cursor, which would change the timestamp but would not necessarily invalidate our computation

[FlorianKroiss]

What about registering a org.eclipse.jface.text.DocumentEvent and then cancelling only if documentAboutToBeChanged/documentChanged is called with an event that indicates that text have been remod from the document? Would that work?

I guess we could track the DocumentEvents to set a flag and then use something like

catch(BadLocationException ex){
if (!computationShouldBeStopped){
    // Log exception
}
}

But I think the extra bookkeeping is not worth it.

I still believe that the catch (BadLocationException ex) blocks that I changed in the PR can only happen if we are in case 2, so a comment why we ignore these should suffice.

[FlorianKroiss]

Fixed by #1486