[PATCH] D50502: [clangd] Initial cancellation mechanism for LSP requests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I have left a few comments, but wanted to start with a higher-level design consideration.

I believe we should probably expose the cancellations in the ClangdServer API directly.
The reasons for that are:

1. We have an internal client that would also benefit from using the cancellation API and we should try to share as much code as possible between that and the rest of clangd.  Creating the cancellation cookies, stashing them in the context are all details that could be handled by ClangdServer so two clients don't have to do the same things.
2. `ClangdServer` seems like a natural layer for adding this, since it's a user-facing **async** API. It spawns async tasks, so it is a perfect place to provide a way to cancel the spawned tasks as well.

The concrete API that I propose is:

- Relevant methods of ClangdServer (we could start we code completion) that spawn async reqeuests returns a cancellation object that allows to cancel the spawned job:

  class TaskHandle {
    void cancel();
  };
  
  class ClangdServer {
    TaskHandle codeComplete(File, Position, Callback);
  };

- Users of the API are free to ignore the `TaskHandle` if they don't need to cancel the corresponding requests.
- If the users are capable of cancelling the request, they are responsible for keeping the `TaskHandle` around before the corresponding callback is called.
- If the request was cancelled, the `Callback` might receive the `TaskCancelledError` in the callback and should handle it accordingly.

The rest of the design LG, i.e.  the code that can actually be cancelled is responsible for checking `CancellationHandler::HasCancelled` and should return a corresponding error in the callback if it was cancelled.



================
Comment at: clangd/Cancellation.h:1
+//===--- Cancellation.h -------------------------------------------*-C++-*-===//
+//
----------------
This file could use some comments about the API and samples of how it can be used.
Could probably be done after we finalize the design, though.


================
Comment at: clangd/Cancellation.h:22
+public:
+  static bool HasCancelled();
+  static Context LLVM_NODISCARD SetCurrentCancellationToken(
----------------
I think we might want to expose the context key for the cancellation primitive here.
The reason is that checking if request is cancelled is a common operation (i.e. I can imagine it being called on every few parsed ast node, etc.), so we'd want to keep the overhead of checking for cancellation very low.

Exposing a key in the context would allow to avoid doing the lookups in the context every time we need to check if request was cancelled.

Note that exposing `shared_ptr<atomic<bool>>` is probably not a great idea, since it'll allow to write into that object too. I suggest we go with a class that wraps it and only allows to read the value of the variable, i.e. only has `isCancelled` method.


================
Comment at: clangd/Cancellation.h:23
+  static bool HasCancelled();
+  static Context LLVM_NODISCARD SetCurrentCancellationToken(
+      std::shared_ptr<std::atomic<bool>> CancellationToken);
----------------
The `LLVM_NODISCARD` is misplaced. The current code applies the attribute to the type (i.e. `Context`), and we want to apply to the function.
There are two ways to do that:
1. `LLVM_NODISCARD static Context SetCurrentCancellationToken()`
2. `static Context SetCurrentCancellationToken LLVM_NODISCARD ()`

The first one is definitely less awkward and more widely used, so I suggest we stick to it


================
Comment at: clangd/Cancellation.h:25
+      std::shared_ptr<std::atomic<bool>> CancellationToken);
+  static llvm::Error GetCancellationError();
+};
----------------
NIT: use `lowerCamelCase` for function names (per [LLVM style guide](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly))


================
Comment at: clangd/Cancellation.h:36
+  std::error_code convertToErrorCode() const override {
+    llvm_unreachable("Tried to get error code on TaskCancelledError");
+  }
----------------
It seems the contract for `ErrorInfo` actually requires this function to be implemented, so using `llvm_unreachable` doesn't seem right here.
We could get away without actually unique error codes by returning `llvm::inconvertibleErrorCode()`, though.


================
Comment at: clangd/ClangdLSPServer.cpp:76
+  return ID.kind() == json::Value::Number
+             ? utostr(static_cast<int64_t>(ID.getAsNumber().getValue()))
+             : std::string(ID.getAsString().getValue());
----------------
1. Maybe use `getAsInteger()` to convert directly to `int64_t`?
2. Maybe use `itostr` to avoid conversion from `int64` to `uint64`, which is undefined for negative numbers?
3. What if input is neither string nor number? Maybe add an assertion and check at the call-sites? That's a form of user input and we don't want clangd to crash in case of incorrect ones.

Alternatively, could we simply pipe the `json::Value` to a `raw_string_stream` and get the pretty-printed string? Would avoid dealing with input validation issues.


================
Comment at: clangd/ClangdLSPServer.h:170
 
+  // Holds cancellation tokens for requests.
+  llvm::StringMap<std::shared_ptr<std::atomic<bool>>> CancellationTokens;
----------------
Maybe clarify in the comment that the key of the map is a serialized request-id?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502