Thank you for this PR! 👍

I quite like the design in general, but I'd assume that calling cancelRequest on a dependent future should also cancel the root future. However, it does not seem to be the case. If I add assertTrue(original.isCancelled()); to the end of testCancellationAfterTransformation, the test fails. Am I missing something?

I made it optional. default behavior is to send the cancel request to the LSP server an wait for the cancel response which gracefully cancels the root future and it's depending stages. using cancelRequest(...,true) you can now force immediate cancellation of the root future and the chain without waiting for the cancellation response. WDYT?

Makes sense to me, thanks a lot for the improvements!

Just a thought (I'm not requesting any change now, just would like to to discuss it with you): the Javadoc for theCompletableFuture.cancel method mentions that its parameter mayInterruptIfRunning has no effect. Therefore, would not it be reasonable to omit this parameter in cancelRequest methods?

Another thought: it might probably make sense to allow for optional cancellation of a dependent future when calling cancelRequest on it just as we allow for optional cancellation of the root future.

These thoughts together lead me to the following design proposal. What if JsonRpcRequestFuture would have getRoot() method that returns the root future, and cancelRequest() method (without any parameters) that sends the cancel notification exactly once for the request, but does not cancel any future. Calling cancel on the root future shall still ensure that cancelRequest is called.

It seems that this design can simplify the JsonRpcRequestFuture API without losing flexibility.

Use cases:

• Fast-cancel: future.getRoot().cancel(true)

• Wait-for-remote: future.cancelRequest() (with optionally calling future.cancel(true) on the dependent future in addition to cancelRequest, though it seems that in most cases it should not be necessary, since in this case fast-cancel should probably be used instead).

I changed it accordingly.

future.cancelRequest() now will only send the $/cancelRequest but not immediately cancel any futures locally. Instead the future will be cancelled indirectly via processing of the cancel response from the server.

Additionally root.cancel(...) will call this.cancelRequest() + super.cancel(...).

@sebthom Thank you very much for all the effort!

I've added a separate test to better isolate the fast-cancel case and also inlined the private methods in JsonRpcRequestFuture (it seemed to me that they didn't add much value to stand on their own).

Please have a look and if you are fine with my updates, I'll be happy to approve and merge this PR. Thanks again!

LGTM! Thanks for the conversation!

Great! Thank you! 👍

Just a note that I'm looking at DebugIntegrationTest.testCancellation, which failed once upon merging this PR:
https://github.com/eclipse-lsp4j/lsp4j/actions/runs/18980903518/job/54212884068

I does not fail locally upon many re-runs (on macOS), but something seems to be flaky.

Unfortunately, I don't have much time right now, but will return to it, probably tomorrow.

@pisv I addressed this via #909

@sebthom Thanks for looking into the issue.

As far as I understand, attempting to cancel a request is inherently racy with regard to the request completion. For example, the request might complete before the cancel notification arrives. This is normal, and should be expected.

Actually, attempting to cancel an ordinary CompletableFuture is also inherently racy with regard to the future completion.

Therefore, I can see no problem with the code of this PR in this regard. In other words, I don't think that the changes proposed in #909 are really necessary or even desired.

Perhaps the test itself expects too much, let's try to take a deeper look at it.

Yes, the problem has been lurking in DebugIntegrationTest.testCancellation and is not related to this PR.

To verify it, check out 4e0cc84 (the last commit before merging this PR), and insert Thread.sleep(500) between the following two lines:

The test will consistently fail after that.

To fix it, the test should expect not only CancellationException, but also ExecutionException caused by ResponseErrorException with message "The request (id: 1, method: 'askClient') has been cancelled".

I'll submit a PR with the fix.