[clang-tools-extra] 31db1e0 - [clangd] Send the correct error code when cancelling requests.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 13 10:42:48 PDT 2020
Author: Sam McCall
Date: 2020-04-13T19:42:38+02:00
New Revision: 31db1e0bd1ea020046eb7ac2359ced8c7239de1e
URL: https://github.com/llvm/llvm-project/commit/31db1e0bd1ea020046eb7ac2359ced8c7239de1e
DIFF: https://github.com/llvm/llvm-project/commit/31db1e0bd1ea020046eb7ac2359ced8c7239de1e.diff
LOG: [clangd] Send the correct error code when cancelling requests.
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
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Cancellation.cpp b/clang-tools-extra/clangd/Cancellation.cpp
index ccd27a1452d1..2120e700e719 100644
--- a/clang-tools-extra/clangd/Cancellation.cpp
+++ b/clang-tools-extra/clangd/Cancellation.cpp
@@ -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
diff --git a/clang-tools-extra/clangd/Cancellation.h b/clang-tools-extra/clangd/Cancellation.h
index bdb0bd4ef09f..0bee6f355c39 100644
--- a/clang-tools-extra/clangd/Cancellation.h
+++ b/clang-tools-extra/clangd/Cancellation.h
@@ -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.";
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index eafe353bb70e..f8e1d1f8bd3f 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -409,7 +409,8 @@ class ClangdLSPServer::MessageHandler : public Transport::MessageHandler {
// - 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.
{
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
index c1773258554c..766cf7642155 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -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) {
diff --git a/clang-tools-extra/clangd/JSONTransport.cpp b/clang-tools-extra/clangd/JSONTransport.cpp
index d82dca167f3b..a9254732562b 100644
--- a/clang-tools-extra/clangd/JSONTransport.cpp
+++ b/clang-tools-extra/clangd/JSONTransport.cpp
@@ -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();
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index 968b039c62cc..d43522002cf0 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -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> {
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 6b2d3368578c..382cfe38239f 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -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,
diff --git a/clang-tools-extra/clangd/unittests/CancellationTests.cpp b/clang-tools-extra/clangd/unittests/CancellationTests.cpp
index 3fe71901a206..09f980748f91 100644
--- a/clang-tools-extra/clangd/unittests/CancellationTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CancellationTests.cpp
@@ -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) {
diff --git a/clang-tools-extra/clangd/unittests/JSONTransportTests.cpp b/clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
index 3f71a10c62fa..2a167bd8fb3c 100644
--- a/clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
+++ b/clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
@@ -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 @@ class Echo : public Transport::MessageHandler {
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"
diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
index bacd1f6b5d98..961a798a53ae 100644
--- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -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(
More information about the cfe-commits
mailing list