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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly NITs, but please take a look at the `CancelParams` handling problem. I believe there might be a potential bug hiding there :-)



================
Comment at: clangd/Cancellation.h:87
+/// Enables async tasks to check for cancellation signal, contains a read only
+/// token cached from context. Can be created with clangd::isCancelled. It is
+/// thread-safe, multiple threads can check for cancellation on the same token.
----------------
s/Can be created with `clangd::isCancelled()`/Tokens for the currently running task can be obtained via clangd::getCurrentCancellationToken/?
(to focus on which kind of tokens can be obtained from the tokens)

Another idea: maybe add a reference no why someone would want to use `CancellationToken` instead of `isCancelled()` helper, e.g.
```
/// If cancellation checks are rare, one could use the isCancelled() helper to simplify the code.
/// However, if cancellation checks are frequent, the guideline is first obtain the CancellationToken for the currently running task with getCurrentCancellationToken() and do cancel checks using it to avoid extra lookups in the Context
```


================
Comment at: clangd/Cancellation.h:137
+  void log(llvm::raw_ostream &OS) const override {
+    OS << "Task got cancelled.";
+  }
----------------
s/got/was?


================
Comment at: clangd/ClangdLSPServer.cpp:621
+  std::lock_guard<std::mutex> Lock(TaskHandlesMutex);
+  const auto &it = TaskHandles.find(Params.ID);
+  if (it != TaskHandles.end()) {
----------------
Wouldn't it work incorrectly for string IDs?
When normalizing `json::Value` from `getRequestID`, we simply print out json. For strings, this should result in quoted output, e.g. `"req"`. However, when parsing `CancelParams`, we parse this json. So we'll end up inserting with a key `"req"` and erasing with a key 'req' (without the quotes).


================
Comment at: clangd/ClangdLSPServer.cpp:643
+  std::lock_guard<std::mutex> Lock(TaskHandlesMutex);
+  TaskHandles.insert({NormalizedID, std::move(TH)});
+}
----------------
Could we `elog` if `insert` indicated that a value already exists in a map? This would probably mean malformed user input, but could also be immensely helpful for debugging.


================
Comment at: clangd/ClangdLSPServer.h:179
+  // associated with only their thread.
+  void CleanupTaskHandle();
+  void StoreTaskHandle(TaskHandle TH);
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > These two methods look like a nice suit for better wrapped in a RAII-object. It would add the value to the associated map on construction and remove on destruction.
> > One problem is that all operations calls would become weird, because we'll have to move this RAII object around and moving into lambdas is not fun without C++14 (which allows `[x=std::move(y)]() {}`..
> > 
> > Maybe add a comment that calls to this function should always be paired and a FIXME that we should have an API that enforces this?
> > 
> Yeah that would be really nice to have them in a RAII object, but the thing is cleanup can occur only after the task dies. Which we don't directly know since threads are being detached. So the only way to trigger destruction is at the callback, which is where I call cleanup currently. Maybe we can store the object within context to trigger destructor at the end of the thread, but still don't think it would look any better or ease the job of the caller.
Having these paired method calls is probably fine for the first iteration, but we should abstract away this pattern when supporting cancellations for more operations.
LG with a FIXME.


================
Comment at: clangd/ClangdServer.cpp:152
 
+  auto CancellableTaskHandle = TaskHandle::create();
+  auto CT = CancellableTaskHandle.createCancellationToken();
----------------
Maybe use a shorter name? E.g. `Task` or `TH` :-)


================
Comment at: clangd/ClangdServer.cpp:153
+  auto CancellableTaskHandle = TaskHandle::create();
+  auto CT = CancellableTaskHandle.createCancellationToken();
   // Copy PCHs to avoid accessing this->PCHs concurrently
----------------
Instead of moving cancellation token around explicitly, we could stash in a context right away and it would nicely propagate into the thread. This looks like less boilerplate.
E.g.
```
  auto CancellableTaskHandle = TaskHandle::create();
  WithContext ContextWithCancellation(setCurrentCancellationToken(TaskHandle.createCancellationToken()));

  auto Task = [](File, Callback<CodeCompleteResult> CB, llvm::Expected<InputsAndPreamble> IP) {
    if (getCancellationToken().isCancelled())
      return CB(make_error<CancelledError>());
   };
   /// rest of the code is the same
```


================
Comment at: clangd/ClangdServer.cpp:178
 
-  WorkScheduler.runWithPreamble("CodeComplete", File,
-                                Bind(Task, File.str(), std::move(CB)));
+  WorkScheduler.runWithPreamble(
+      "CodeComplete", File,
----------------
Thinking out loud: the TaskHandle and CancellationToken handling will probably eventually move into `TUScheduler::runWithPreamble` and other thread-spawning code to avoid this kind of boilerplate here.

No need to change anything in this patch, though.


================
Comment at: clangd/Protocol.h:871
+struct CancelParams {
+  std::string ID;
+};
----------------
Maybe add a comment from the LSP spec to be consistent with the rest of the file?
Also note that this can be either a number or a string, but we choose to print out a number in that case and just always use strings.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50502