[clangd] Send the correct error code when cancelling requests.
authorSam McCall <sam.mccall@gmail.com>
Sat, 11 Apr 2020 16:19:50 +0000 (18:19 +0200)
committerSam McCall <sam.mccall@gmail.com>
Mon, 13 Apr 2020 17:42:38 +0000 (19:42 +0200)
Summary:
I couldn't quite bring myself to make Cancellation depend on LSP ErrorCode.
Magic numbers instead...

Reviewers: kadircet

Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, jfb, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D77947

clang-tools-extra/clangd/Cancellation.cpp
clang-tools-extra/clangd/Cancellation.h
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/JSONTransport.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/unittests/CancellationTests.cpp
clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

index ccd27a1..2120e70 100644 (file)
@@ -16,27 +16,28 @@ char CancelledError::ID = 0;
 
 // We don't want a cancelable scope to "shadow" an enclosing one.
 struct CancelState {
-  std::shared_ptr<std::atomic<bool>> Cancelled;
+  std::shared_ptr<std::atomic<int>> Cancelled;
   const CancelState *Parent;
 };
 static Key<CancelState> StateKey;
 
-std::pair<Context, Canceler> cancelableTask() {
+std::pair<Context, Canceler> cancelableTask(int Reason) {
+  assert(Reason != 0 && "Can't detect cancellation if Reason is zero");
   CancelState State;
-  State.Cancelled = std::make_shared<std::atomic<bool>>();
+  State.Cancelled = std::make_shared<std::atomic<int>>();
   State.Parent = Context::current().get(StateKey);
   return {
       Context::current().derive(StateKey, State),
-      [Flag(State.Cancelled)] { *Flag = true; },
+      [Reason, Flag(State.Cancelled)] { *Flag = Reason; },
   };
 }
 
-bool isCancelled(const Context &Ctx) {
+int isCancelled(const Context &Ctx) {
   for (const CancelState *State = Ctx.get(StateKey); State != nullptr;
        State = State->Parent)
-    if (State->Cancelled->load())
-      return true;
-  return false;
+    if (int Reason = State->Cancelled->load())
+      return Reason;
+  return 0;
 }
 
 } // namespace clangd
index bdb0bd4..0bee6f3 100644 (file)
@@ -71,20 +71,24 @@ using Canceler = std::function<void()>;
 
 /// Defines a new task whose cancellation may be requested.
 /// The returned Context defines the scope of the task.
-/// When the context is active, isCancelled() is false until the Canceler is
-/// invoked, and true afterwards.
-std::pair<Context, Canceler> cancelableTask();
+/// When the context is active, isCancelled() is 0 until the Canceler is
+/// invoked, and equal to Reason afterwards.
+/// Conventionally, Reason may be the LSP error code to return.
+std::pair<Context, Canceler> cancelableTask(int Reason = 1);
 
-/// True if the current context is within a cancelable task which was cancelled.
+/// If the current context is within a cancelled task, returns the reason.
 /// (If the context is within multiple nested tasks, true if any are cancelled).
-/// Always false if there is no active cancelable task.
+/// Always zero if there is no active cancelable task.
 /// This isn't free (context lookup) - don't call it in a tight loop.
-bool isCancelled(const Context &Ctx = Context::current());
+int isCancelled(const Context &Ctx = Context::current());
 
 /// Conventional error when no result is returned due to cancellation.
 class CancelledError : public llvm::ErrorInfo<CancelledError> {
 public:
   static char ID;
+  const int Reason;
+
+  CancelledError(int Reason) : Reason(Reason) {}
 
   void log(llvm::raw_ostream &OS) const override {
     OS << "Task was cancelled.";
index eafe353..f8e1d1f 100644 (file)
@@ -409,7 +409,8 @@ private:
   //  - cleans up the entry in RequestCancelers when it's no longer needed
   // If a client reuses an ID, the last wins and the first cannot be canceled.
   Context cancelableRequestContext(const llvm::json::Value &ID) {
-    auto Task = cancelableTask();
+    auto Task = cancelableTask(
+        /*Reason=*/static_cast<int>(ErrorCode::RequestCancelled));
     auto StrID = llvm::to_string(ID);  // JSON-serialize ID for map key.
     auto Cookie = NextRequestCookie++; // No lock, only called on main thread.
     {
index c177325..766cf76 100644 (file)
@@ -214,8 +214,8 @@ void ClangdServer::codeComplete(PathRef File, Position Pos,
                this](llvm::Expected<InputsAndPreamble> IP) mutable {
     if (!IP)
       return CB(IP.takeError());
-    if (isCancelled())
-      return CB(llvm::make_error<CancelledError>());
+    if (auto Reason = isCancelled())
+      return CB(llvm::make_error<CancelledError>(Reason));
 
     llvm::Optional<SpeculativeFuzzyFind> SpecFuzzyFind;
     if (!IP->Preamble) {
index d82dca1..a925473 100644 (file)
@@ -5,6 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
+#include "Cancellation.h"
 #include "Logger.h"
 #include "Protocol.h" // For LSPError
 #include "Shutdown.h"
@@ -22,7 +23,21 @@ llvm::json::Object encodeError(llvm::Error E) {
   // FIXME: encode cancellation errors using RequestCancelled or ContentModified
   // as appropriate.
   if (llvm::Error Unhandled = llvm::handleErrors(
-          std::move(E), [&](const LSPError &L) -> llvm::Error {
+          std::move(E),
+          [&](const CancelledError &C) -> llvm::Error {
+            switch (C.Reason) {
+            case static_cast<int>(ErrorCode::ContentModified):
+              Code = ErrorCode::ContentModified;
+              Message = "Request cancelled because the document was modified";
+              break;
+            default:
+              Code = ErrorCode::RequestCancelled;
+              Message = "Request cancelled";
+              break;
+            }
+            return llvm::Error::success();
+          },
+          [&](const LSPError &L) -> llvm::Error {
             Message = L.Message;
             Code = L.Code;
             return llvm::Error::success();
index 968b039..d435220 100644 (file)
@@ -50,6 +50,7 @@ enum class ErrorCode {
 
   // Defined by the protocol.
   RequestCancelled = -32800,
+  ContentModified = -32801,
 };
 // Models an LSP error as an llvm::Error.
 class LSPError : public llvm::ErrorInfo<LSPError> {
index 6b2d336..382cfe3 100644 (file)
@@ -648,8 +648,8 @@ void ASTWorker::runWithAST(
     llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
     TUScheduler::ASTActionInvalidation Invalidation) {
   auto Task = [=, Action = std::move(Action)]() mutable {
-    if (isCancelled())
-      return Action(llvm::make_error<CancelledError>());
+    if (auto Reason = isCancelled())
+      return Action(llvm::make_error<CancelledError>(Reason));
     llvm::Optional<std::unique_ptr<ParsedAST>> AST = IdleASTs.take(this);
     if (!AST) {
       StoreDiags CompilerInvocationDiagConsumer;
@@ -955,7 +955,8 @@ void ASTWorker::startTask(llvm::StringRef Name,
     Canceler Invalidate = nullptr;
     if (Invalidation) {
       WithContext WC(std::move(Ctx));
-      std::tie(Ctx, Invalidate) = cancelableTask();
+      std::tie(Ctx, Invalidate) = cancelableTask(
+          /*Reason=*/static_cast<int>(ErrorCode::ContentModified));
     }
     Requests.push_back({std::move(Task), std::string(Name), steady_clock::now(),
                         std::move(Ctx), UpdateType, Invalidation,
index 3fe7190..09f9807 100644 (file)
@@ -45,12 +45,13 @@ TEST(CancellationTest, TaskContextDiesHandleLives) {
 }
 
 struct NestedTasks {
+  enum { OuterReason = 1, InnerReason = 2 };
   std::pair<Context, Canceler> Outer, Inner;
   NestedTasks() {
-    Outer = cancelableTask();
+    Outer = cancelableTask(OuterReason);
     {
       WithContext WithOuter(Outer.first.clone());
-      Inner = cancelableTask();
+      Inner = cancelableTask(InnerReason);
     }
   }
 };
@@ -59,13 +60,13 @@ TEST(CancellationTest, Nested) {
   // Cancelling inner task works but leaves outer task unaffected.
   NestedTasks CancelInner;
   CancelInner.Inner.second();
-  EXPECT_TRUE(isCancelled(CancelInner.Inner.first));
+  EXPECT_EQ(NestedTasks::InnerReason, isCancelled(CancelInner.Inner.first));
   EXPECT_FALSE(isCancelled(CancelInner.Outer.first));
   // Cancellation of outer task is inherited by inner task.
   NestedTasks CancelOuter;
   CancelOuter.Outer.second();
-  EXPECT_TRUE(isCancelled(CancelOuter.Inner.first));
-  EXPECT_TRUE(isCancelled(CancelOuter.Outer.first));
+  EXPECT_EQ(NestedTasks::OuterReason, isCancelled(CancelOuter.Inner.first));
+  EXPECT_EQ(NestedTasks::OuterReason, isCancelled(CancelOuter.Outer.first));
 }
 
 TEST(CancellationTest, AsynCancellationTest) {
index 3f71a10..2a167bd 100644 (file)
@@ -5,6 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
+#include "Cancellation.h"
 #include "Protocol.h"
 #include "Transport.h"
 #include "gmock/gmock.h"
@@ -70,6 +71,9 @@ public:
     if (Method == "err")
       Target.reply(
           ID, llvm::make_error<LSPError>("trouble at mill", ErrorCode(88)));
+    else if (Method == "invalidated") // gone out skew on treadle
+      Target.reply(ID, llvm::make_error<CancelledError>(
+                           static_cast<int>(ErrorCode::ContentModified)));
     else
       Target.reply(ID, std::move(Params));
     return true;
@@ -140,7 +144,7 @@ TEST_F(JSONTransportTest, DelimitedPretty) {
 ---
 {"jsonrpc": "2.0", "id": "xyz", "error": {"code": 99, "message": "bad!"}}
 ---
-{"jsonrpc": "2.0", "method": "err", "id": "wxyz", "params": "boom!"}
+{"jsonrpc": "2.0", "method": "invalidated", "id": "wxyz", "params": "boom!"}
 ---
 {"jsonrpc": "2.0", "method": "exit"}
   )jsonrpc",
@@ -154,7 +158,7 @@ Notification call: 1234
 Reply(1234): 5678
 Call foo("abcd"): "efgh"
 Reply("xyz"): error = 99: bad!
-Call err("wxyz"): "boom!"
+Call invalidated("wxyz"): "boom!"
 Notification exit: null
   )";
   EXPECT_EQ(trim(E.log()), trim(WantLog));
@@ -171,11 +175,11 @@ Notification exit: null
   "jsonrpc": "2.0",
   "result": "efgh"
 })"
-                           "Content-Length: 105\r\n\r\n"
+                           "Content-Length: 145\r\n\r\n"
                            R"({
   "error": {
-    "code": 88,
-    "message": "trouble at mill"
+    "code": -32801,
+    "message": "Request cancelled because the document was modified"
   },
   "id": "wxyz",
   "jsonrpc": "2.0"
index bacd1f6..961a798 100644 (file)
@@ -415,7 +415,9 @@ TEST_F(TUSchedulerTests, Invalidation) {
         EXPECT_FALSE(bool(AST));
         llvm::Error E = AST.takeError();
         EXPECT_TRUE(E.isA<CancelledError>());
-        consumeError(std::move(E));
+        handleAllErrors(std::move(E), [&](const CancelledError &E) {
+          EXPECT_EQ(E.Reason, static_cast<int>(ErrorCode::ContentModified));
+        });
       },
       TUScheduler::InvalidateOnUpdate);
   S.runWithAST(