sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, usaxena95, jfb, arphaman, jkorous,
MaskRay, javed.absar, ilya-biryukov.
Herald added a project: clang.
I couldn't quite bring myself to make Cancellation depend on LSP ErrorCode.
Magic numbers instead...
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D77947
Files:
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: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -414,7 +414,9 @@
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(
Index: clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
+++ 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 @@
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 @@
---
{"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 @@
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 @@
"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: clang-tools-extra/clangd/unittests/CancellationTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CancellationTests.cpp
+++ clang-tools-extra/clangd/unittests/CancellationTests.cpp
@@ -45,12 +45,13 @@
}
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 @@
// 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: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -648,8 +648,8 @@
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 @@
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: clang-tools-extra/clangd/Protocol.h
===================================================================
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -50,6 +50,7 @@
// Defined by the protocol.
RequestCancelled = -32800,
+ ContentModified = -32801,
};
// Models an LSP error as an llvm::Error.
class LSPError : public llvm::ErrorInfo<LSPError> {
Index: clang-tools-extra/clangd/JSONTransport.cpp
===================================================================
--- clang-tools-extra/clangd/JSONTransport.cpp
+++ 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 @@
// 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: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -214,8 +214,8 @@
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: clang-tools-extra/clangd/ClangdLSPServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -409,7 +409,8 @@
// - 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: clang-tools-extra/clangd/Cancellation.h
===================================================================
--- clang-tools-extra/clangd/Cancellation.h
+++ clang-tools-extra/clangd/Cancellation.h
@@ -71,20 +71,24 @@
/// 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: clang-tools-extra/clangd/Cancellation.cpp
===================================================================
--- clang-tools-extra/clangd/Cancellation.cpp
+++ clang-tools-extra/clangd/Cancellation.cpp
@@ -16,27 +16,28 @@
// 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
