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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for missing this one. The biggest concern that I have is about the thread-safety of the API, it's too easy to misuse from multiple threads at this point.

We should define thread-safety guarantees for `TaskHandle` and `CancellationToken` and make sure they're not easily broken. The suggested way is to make both move-only types.
For `CancellationToken` we might need a non-thread-safe copy operation, though. But maybe make it a separate method (`clone`?) with a clear statement that it's not thread-safe?

Other than that just NITs.



================
Comment at: clangd/Cancellation.cpp:17
+namespace {
+static Key<std::shared_ptr<std::atomic<bool>>> CancellationTokenKey;
+} // namespace
----------------
Having a `shared_ptr` key in the Context can cause data races (e.g. if we copy it concurrently from multiple threads).
I suggest we make `CancellationToken` move-only (i.e. disallow copies) and return `const CancellationToken&` when getting it from the context.


================
Comment at: clangd/Cancellation.cpp:34
+llvm::Error CancellationHandler::getCancellationError() {
+  return llvm::make_error<TaskCancelledError>();
+}
----------------
I'm not sure if this function buys us much in terms of clarity, maybe remove it and inline everywhere?


================
Comment at: clangd/Cancellation.h:9
+//===----------------------------------------------------------------------===//
+// CancellationToken mechanism for async threads. The caller can generate a
+// TaskHandle for cancellable tasks, then bind that handle to current context
----------------
The comments seems to mention all the important bits, but maybe we could separate different concerns more clearly based on how the code should be used?
E.g. here's a rough draft to illustrate the idea:

```
Cancellation mechanism for async tasks. Roughly all the clients of this code can be classified into three categories:
1. The code that creates and schedules async tasks, e.g. TUScheduler.
2. The callers of the async method that can cancel some of the running tasks, e.g. `ClangdLSPServer`
3. The code running inside the async task itself, i.e. code completion or find definition implementation that run clang, etc.

For (1), the guideline is to accept a callback for the result of async operation and return a `TaskHandle` to allow cancelling the request.
    TaskHandle someAsyncMethod(function<void(llvm::Expected<ResultType>)> Callback);

The callers of async methods (2) can issue cancellations and should be prepared to handle `TaskCancelledError` result:
    TaskHandle H = someAsyncMethod([](llvm::Expected<ResultType> R) {
        if (/* code to check for TaskCancelledError */)
           llvm::errs() << "Task got cancelled";
         else
           llvm::errs() << "The result is: " << *R;        
        } 
    });

    sleep(5);
    H.cancel();
    
The worker code itself (3) should check for cancellations using `CancellationToken` that can be retrieved via `CancellationToken::isCancelled()`.
    void codeComplete() {
      const CancellationToken& CT = CancellationToken::isCancelled();
      if (CT.isCancelled()) 
        return llvm::makeError<TaskCancelledError>();

      runSema();
      if (CT.isCancelled()) 
        return llvm::makeError<TaskCancelledError>();

      queryIndex();  
      return results;
    }
If the operation was cancelled before it could run to completion, it should propagate the TaskCancelledError as a result.
```


================
Comment at: clangd/Cancellation.h:60
+
+class CancellationToken {
+private:
----------------
We're missing docs on this and the other classes. I suggest a small doc describing what they're used for, e.g. `used for checking if the operation was cancelled` and `used to signal the operation should be cancelled`, etc.

Also noting the threading-safety guarantees is a probably a good idea.


================
Comment at: clangd/Cancellation.h:61
+class CancellationToken {
+private:
+  std::shared_ptr<const std::atomic<bool>> Token;
----------------
NIT: move members to the end of the class to be consistent with the rest of clangd.


================
Comment at: clangd/Cancellation.h:67
+  operator bool() const { return isCancelled(); }
+  CancellationToken(const std::shared_ptr<const std::atomic<bool>> Token)
+      : Token(Token) {}
----------------
Can we make this ctor private? 


================
Comment at: clangd/Cancellation.h:71
+
+class TaskHandle {
+public:
----------------
I wonder if we should make `TaskHandle` move-only?

The reasoning is that is can be easily used from multiple threads (since it's used by code dealing with async tasks anyway) and copying it concurrently is a data race.
On the other hand, calling `cancel()` is perfectly thread-safe and shouldn't cause any problems.


================
Comment at: clangd/Cancellation.h:74
+  void cancel();
+  static TaskHandle createCancellableTaskHandle();
+  friend class CancellationHandler;
----------------
NIT: maybe use a shorter name, e.g. `TaskHandle::create`?


================
Comment at: clangd/Cancellation.h:82
+
+class CancellationHandler {
+public:
----------------
Maybe remove `CancellationHandler` and put all its functions into the clangd namespace? It's somewhat easily confused with `CancellationToken`.
We could declare some function friends if we need to


================
Comment at: clangd/Cancellation.h:89
+
+class TaskCancelledError : public llvm::ErrorInfo<TaskCancelledError> {
+public:
----------------
NIT: maybe use a shorter name: `CancelledError`?


================
Comment at: clangd/ClangdLSPServer.cpp:355
+          Error Uncaught =
+              handleErrors(List.takeError(), [](const TaskCancelledError &) {
+                replyError(ErrorCode::RequestCancelled,
----------------
I expect the pattern `if cancelled -> reply with ErrorCode::RequestCancelled else reply with ErrorCode::InvalidParams` to be pretty common when we add cancellation to more operations.

Maybe add a small helper that does that? E.g.
```
/// Replies to a request with an error, passing proper error codes to the LSP client.
/// Properly handles cancellations.
void replyError(Error E);
```


================
Comment at: clangd/ClangdLSPServer.cpp:627
+void ClangdLSPServer::onCancelRequest(CancelParams &Params) {
+  std::lock_guard<std::mutex> _(TaskHandlesMutex);
+  const auto &it = TaskHandles.find(Params.ID);
----------------
NIT: use `Lock` or `L` for consistencty with the rest of clangd. it's somewhat easy to misread `std::lock_guard<std::mutex> _(Mut)`  as `std::lock_guard<std::mutex> (Mut)`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502