[PATCH] D98414: [clangd] Turn off implicit cancellation based on client capabilities

Thu Mar 11 09:28:37 PST 2021

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!

================
Comment at: clang-tools-extra/clangd/ClangdServer.h:153
+    /// to cancel. Clients that always cancel stale requests should clear this.
+    bool ImplicitCancellation = true;
+
----------------
sammccall wrote:
> kadircet wrote:
> > this makes sense as is, but i wonder if we should lift this to LSPServer instead.
> > 
> > 3.17 specs also introduce `retryOnContentModified`, which is supposed to be a list of RPC names. so if we decide to take that into account, rather than deciding on what's "transient" ourselves, having all of this hardcoded in clangdserver would be limiting. 
> > 
> > We can accept a cancellation policy on all of the ClangdServer endpoints, have some sensible defaults in clangdlspserver, enable those to be overwritten by client specs on initialize. (or if we don't want to make them part of the signature i suppose we can make use of config/context, but ... yikes). WDYT?
> I thought about this, but I don't think it's better...
> 
>  - it's complicated and boilerplatey with the method names, way out of proportion to the value here
>  - I think "will the client take care of cancellation" is actually the more pertinent question rather than "will the client retry if we cancel"
>  - we want to disable this (entirely) in C++ embedders, and a boolean option is the simplest way to do that.
> we want to disable this (entirely) in C++ embedders, and a boolean option is the simplest way to do that.

i think it should be okay for embedders to specify what behavior they want per action, but i agree with the other two points. thanks for the explanation!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98414/new/

https://reviews.llvm.org/D98414