[PATCH] D52004: [clangd] Allow all LSP methods to signal cancellation via $/cancelRequest

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Wonder if we can still keep the onCancelRequest registry within ProtocolHandler's scope, so that it is clear that we implement it. Other than that seems fascinating, thanks!



================
Comment at: clangd/JSONRPCDispatcher.cpp:246
+  auto StrID = llvm::to_string(ID); // JSON-serialize ID for map key.
+  auto Cookie = RequestCookie++;
+  std::lock_guard<std::mutex> Lock(CancelMutex);
----------------
Maybe we can say something about this method is actually being invoked in a sync manner and the reason why we have mutex below is due to context destruction of the thread we might spawn later on. Because it bugged me at first look not having this line under mutex as well, then I noticed actually this was still a sync function.


================
Comment at: clangd/JSONRPCDispatcher.cpp:248
+  std::lock_guard<std::mutex> Lock(CancelMutex);
+  RequestCancelers[StrID] = {std::move(Task.second), Cookie};
+  // When the request ends, we can clean up the entry we just added.
----------------
maybe limit the Lock's scope just to put element into map.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52004