This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE342130: [clangd] Simplify cancellation public API
(authored by sammccall, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D51996?vs=165158&id=165251#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D51996
Files:
clangd/Cancellation.cpp
clangd/Cancellation.h
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
unittests/clangd/CancellationTests.cpp
Index: unittests/clangd/CancellationTests.cpp
===================================================================
--- unittests/clangd/CancellationTests.cpp
+++ unittests/clangd/CancellationTests.cpp
@@ -13,57 +13,48 @@
namespace {
TEST(CancellationTest, CancellationTest) {
- TaskHandle TH = Task::createHandle();
- WithContext ContextWithCancellation(setCurrentTask(TH));
+ auto Task = cancelableTask();
+ WithContext ContextWithCancellation(std::move(Task.first));
EXPECT_FALSE(isCancelled());
- TH->cancel();
+ Task.second();
EXPECT_TRUE(isCancelled());
}
-TEST(CancellationTest, TaskTestHandleDiesContextLives) {
+TEST(CancellationTest, CancelerDiesContextLives) {
llvm::Optional<WithContext> ContextWithCancellation;
{
- TaskHandle TH = Task::createHandle();
- ContextWithCancellation.emplace(setCurrentTask(TH));
+ auto Task = cancelableTask();
+ ContextWithCancellation.emplace(std::move(Task.first));
EXPECT_FALSE(isCancelled());
- TH->cancel();
+ Task.second();
EXPECT_TRUE(isCancelled());
}
EXPECT_TRUE(isCancelled());
}
TEST(CancellationTest, TaskContextDiesHandleLives) {
- TaskHandle TH = Task::createHandle();
+ auto Task = cancelableTask();
{
- WithContext ContextWithCancellation(setCurrentTask(TH));
+ WithContext ContextWithCancellation(std::move(Task.first));
EXPECT_FALSE(isCancelled());
- TH->cancel();
+ Task.second();
EXPECT_TRUE(isCancelled());
}
// Still should be able to cancel without any problems.
- TH->cancel();
-}
-
-TEST(CancellationTest, CancellationToken) {
- TaskHandle TH = Task::createHandle();
- WithContext ContextWithCancellation(setCurrentTask(TH));
- const auto &CT = getCurrentTask();
- EXPECT_FALSE(CT.isCancelled());
- TH->cancel();
- EXPECT_TRUE(CT.isCancelled());
+ Task.second();
}
TEST(CancellationTest, AsynCancellationTest) {
std::atomic<bool> HasCancelled(false);
Notification Cancelled;
- auto TaskToBeCancelled = [&](ConstTaskHandle CT) {
- WithContext ContextGuard(setCurrentTask(std::move(CT)));
+ auto TaskToBeCancelled = [&](Context Ctx) {
+ WithContext ContextGuard(std::move(Ctx));
Cancelled.wait();
HasCancelled = isCancelled();
};
- TaskHandle TH = Task::createHandle();
- std::thread AsyncTask(TaskToBeCancelled, TH);
- TH->cancel();
+ auto Task = cancelableTask();
+ std::thread AsyncTask(TaskToBeCancelled, std::move(Task.first));
+ Task.second();
Cancelled.notify();
AsyncTask.join();
Index: clangd/Cancellation.h
===================================================================
--- clangd/Cancellation.h
+++ clangd/Cancellation.h
@@ -6,124 +6,82 @@
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
-// Cancellation mechanism for async tasks. Roughly all the clients of this code
-// can be classified into three categories:
-// 1. The code that creates and schedules async tasks, e.g. TUScheduler.
-// 2. The callers of the async method that can cancel some of the running tasks,
-// e.g. `ClangdLSPServer`
-// 3. The code running inside the async task itself, i.e. code completion or
-// find definition implementation that run clang, etc.
-//
-// For (1), the guideline is to accept a callback for the result of async
-// operation and return a `TaskHandle` to allow cancelling the request.
-//
-// TaskHandle someAsyncMethod(Runnable T,
-// function<void(llvm::Expected<ResultType>)> Callback) {
-// auto TH = Task::createHandle();
-// WithContext ContextWithCancellationToken(TH);
-// auto run = [](){
-// Callback(T());
+// Cancellation mechanism for long-running tasks.
+//
+// This manages interactions between:
+//
+// 1. Client code that starts some long-running work, and maybe cancels later.
+//
+// std::pair<Context, Canceler> Task = cancelableTask();
+// {
+// WithContext Cancelable(std::move(Task.first));
+// Expected
+// deepThoughtAsync([](int answer){ errs() << answer; });
// }
-// // Start run() in a new async thread, and make sure to propagate Context.
-// return TH;
-// }
-//
-// The callers of async methods (2) can issue cancellations and should be
-// prepared to handle `TaskCancelledError` result:
-//
-// void Caller() {
-// // You should store this handle if you wanna cancel the task later on.
-// TaskHandle TH = someAsyncMethod(Task, [](llvm::Expected<ResultType> R) {
-// if(/*check for task cancellation error*/)
-// // Handle the error
-// // Do other things on R.
-// });
-// // To cancel the task:
-// sleep(5);
-// TH->cancel();
-// }
-//
-// The worker code itself (3) should check for cancellations using
-// `Task::isCancelled` that can be retrieved via `getCurrentTask()`.
-//
-// llvm::Expected<ResultType> AsyncTask() {
-// // You can either store the read only TaskHandle by calling getCurrentTask
-// // once and just use the variable everytime you want to check for
-// // cancellation, or call isCancelled everytime. The former is more
-// // efficient if you are going to have multiple checks.
-// const auto T = getCurrentTask();
-// // DO SMTHNG...
-// if(T.isCancelled()) {
-// // Task has been cancelled, lets get out.
-// return llvm::makeError<CancelledError>();
-// }
-// // DO SOME MORE THING...
-// if(T.isCancelled()) {
-// // Task has been cancelled, lets get out.
-// return llvm::makeError<CancelledError>();
-// }
-// return ResultType(...);
-// }
-// If the operation was cancelled before task could run to completion, it should
-// propagate the TaskCancelledError as a result.
+// // ...some time later...
+// if (User.fellAsleep())
+// Task.second();
+//
+// (This example has an asynchronous computation, but synchronous examples
+// work similarly - the Canceler should be invoked from another thread).
+//
+// 2. Library code that executes long-running work, and can exit early if the
+// result is not needed.
+//
+// void deepThoughtAsync(std::function<void(int)> Callback) {
+// runAsync([Callback]{
+// int A = ponder(6);
+// if (isCancelled())
+// return;
+// int B = ponder(9);
+// if (isCancelled())
+// return;
+// Callback(A * B);
+// });
+// }
+//
+// (A real example may invoke the callback with an error on cancellation,
+// the CancelledError is provided for this purpose).
+//
+// Cancellation has some caveats:
+// - the work will only stop when/if the library code next checks for it.
+// Code outside clangd such as Sema will not do this.
+// - it's inherently racy: client code must be prepared to accept results
+// even after requesting cancellation.
+// - it's Context-based, so async work must be dispatched to threads in
+// ways that preserve the context. (Like runAsync() or TUScheduler).
+//
+// FIXME: We could add timestamps to isCancelled() and CancelledError.
+// Measuring the start -> cancel -> acknowledge -> finish timeline would
+// help find where libraries' cancellation should be improved.
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CANCELLATION_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CANCELLATION_H
#include "Context.h"
#include "llvm/Support/Error.h"
-#include <atomic>
-#include <memory>
+#include <functional>
#include <system_error>
namespace clang {
namespace clangd {
-/// Enables signalling a cancellation on an async task or checking for
-/// cancellation. It is thread-safe to trigger cancellation from multiple
-/// threads or check for cancellation. Task object for the currently running
-/// task can be obtained via clangd::getCurrentTask().
-class Task {
-public:
- void cancel() { CT = true; }
- /// If cancellation checks are rare, one could use the isCancelled() helper in
- /// the namespace to simplify the code. However, if cancellation checks are
- /// frequent, the guideline is first obtain the Task object for the currently
- /// running task with getCurrentTask() and do cancel checks using it to avoid
- /// extra lookups in the Context.
- bool isCancelled() const { return CT; }
-
- /// Creates a task handle that can be used by an async task to check for
- /// information that can change during it's runtime, like Cancellation.
- static std::shared_ptr<Task> createHandle() {
- return std::shared_ptr<Task>(new Task());
- }
-
- Task(const Task &) = delete;
- Task &operator=(const Task &) = delete;
- Task(Task &&) = delete;
- Task &operator=(Task &&) = delete;
-
-private:
- Task() : CT(false) {}
- std::atomic<bool> CT;
-};
-using ConstTaskHandle = std::shared_ptr<const Task>;
-using TaskHandle = std::shared_ptr<Task>;
-
-/// Fetches current task information from Context. TaskHandle must have been
-/// stashed into context beforehand.
-const Task &getCurrentTask();
-
-/// Stashes current task information within the context.
-LLVM_NODISCARD Context setCurrentTask(ConstTaskHandle TH);
-
-/// Checks whether the current task has been cancelled or not.
-/// Consider storing the task handler returned by getCurrentTask and then
-/// calling isCancelled through it. getCurrentTask is expensive since it does a
-/// lookup in the context.
-inline bool isCancelled() { return getCurrentTask().isCancelled(); }
+/// A canceller requests cancellation of a task, when called.
+/// Calling it again has no effect.
+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();
+
+/// True if the current context is within a cancelable task which was cancelled.
+/// Always false if there is no active cancelable task.
+/// This isn't free (context lookup) - don't call it in a tight loop.
+bool isCancelled();
+/// Conventional error when no result is returned due to cancellation.
class CancelledError : public llvm::ErrorInfo<CancelledError> {
public:
static char ID;
Index: clangd/ClangdLSPServer.cpp
===================================================================
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -348,7 +348,7 @@
void ClangdLSPServer::onCompletion(TextDocumentPositionParams &Params) {
CreateSpaceForTaskHandle();
- TaskHandle TH = Server.codeComplete(
+ Canceler Cancel = Server.codeComplete(
Params.textDocument.uri.file(), Params.position, CCOpts,
[this](llvm::Expected<CodeCompleteResult> List) {
auto _ = llvm::make_scope_exit([this]() { CleanupTaskHandle(); });
@@ -361,7 +361,7 @@
LSPList.items.push_back(R.render(CCOpts));
return reply(std::move(LSPList));
});
- StoreTaskHandle(std::move(TH));
+ StoreTaskHandle(std::move(Cancel));
}
void ClangdLSPServer::onSignatureHelp(TextDocumentPositionParams &Params) {
@@ -635,8 +635,7 @@
const auto &It = TaskHandles.find(Params.ID);
if (It == TaskHandles.end())
return;
- if (It->second)
- It->second->cancel();
+ It->second();
TaskHandles.erase(It);
}
@@ -659,7 +658,7 @@
elog("Creation of space for task handle: {0} failed.", NormalizedID);
}
-void ClangdLSPServer::StoreTaskHandle(TaskHandle TH) {
+void ClangdLSPServer::StoreTaskHandle(Canceler TH) {
const json::Value *ID = getRequestId();
if (!ID)
return;
Index: clangd/ClangdLSPServer.h
===================================================================
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -170,9 +170,9 @@
// destructed instance of ClangdLSPServer.
ClangdServer Server;
- // Holds task handles for running requets. Key of the map is a serialized
- // request id.
- llvm::StringMap<TaskHandle> TaskHandles;
+ // Holds cancelers for running requets. Key of the map is a serialized
+ // request id. FIXME: handle cancellation generically in JSONRPCDispatcher.
+ llvm::StringMap<Canceler> TaskHandles;
std::mutex TaskHandlesMutex;
// Following three functions are for managing TaskHandles map. They store or
@@ -183,7 +183,7 @@
// request.
void CleanupTaskHandle();
void CreateSpaceForTaskHandle();
- void StoreTaskHandle(TaskHandle TH);
+ void StoreTaskHandle(Canceler TH);
};
} // namespace clangd
} // namespace clang
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -125,9 +125,9 @@
/// while returned future is not yet ready.
/// A version of `codeComplete` that runs \p Callback on the processing thread
/// when codeComplete results become available.
- TaskHandle codeComplete(PathRef File, Position Pos,
- const clangd::CodeCompleteOptions &Opts,
- Callback<CodeCompleteResult> CB);
+ Canceler codeComplete(PathRef File, Position Pos,
+ const clangd::CodeCompleteOptions &Opts,
+ Callback<CodeCompleteResult> CB);
/// Provide signature help for \p File at \p Pos. This method should only be
/// called for tracked files.
Index: clangd/Cancellation.cpp
===================================================================
--- clangd/Cancellation.cpp
+++ clangd/Cancellation.cpp
@@ -13,21 +13,21 @@
namespace clang {
namespace clangd {
-namespace {
-static Key<ConstTaskHandle> TaskKey;
-} // namespace
-
char CancelledError::ID = 0;
+static Key<std::shared_ptr<std::atomic<bool>>> FlagKey;
-const Task &getCurrentTask() {
- const auto TH = Context::current().getExisting(TaskKey);
- assert(TH && "Fetched a nullptr for TaskHandle from context.");
- return *TH;
+std::pair<Context, Canceler> cancelableTask() {
+ auto Flag = std::make_shared<std::atomic<bool>>();
+ return {
+ Context::current().derive(FlagKey, Flag),
+ [Flag] { *Flag = true; },
+ };
}
-Context setCurrentTask(ConstTaskHandle TH) {
- assert(TH && "Trying to stash a nullptr as TaskHandle into context.");
- return Context::current().derive(TaskKey, std::move(TH));
+bool isCancelled() {
+ if (auto *Flag = Context::current().get(FlagKey))
+ return **Flag;
+ return false; // Not in scope of a task.
}
} // namespace clangd
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -201,16 +201,16 @@
WorkScheduler.remove(File);
}
-TaskHandle ClangdServer::codeComplete(PathRef File, Position Pos,
- const clangd::CodeCompleteOptions &Opts,
- Callback<CodeCompleteResult> CB) {
+Canceler ClangdServer::codeComplete(PathRef File, Position Pos,
+ const clangd::CodeCompleteOptions &Opts,
+ Callback<CodeCompleteResult> CB) {
// Copy completion options for passing them to async task handler.
auto CodeCompleteOpts = Opts;
if (!CodeCompleteOpts.Index) // Respect overridden index.
CodeCompleteOpts.Index = Index;
- TaskHandle TH = Task::createHandle();
- WithContext ContextWithCancellation(setCurrentTask(TH));
+ auto Cancelable = cancelableTask();
+ WithContext ContextWithCancellation(std::move(Cancelable.first));
// Copy PCHs to avoid accessing this->PCHs concurrently
std::shared_ptr<PCHContainerOperations> PCHs = this->PCHs;
auto FS = FSProvider.getFileSystem();
@@ -259,7 +259,7 @@
// We use a potentially-stale preamble because latency is critical here.
WorkScheduler.runWithPreamble("CodeComplete", File, TUScheduler::Stale,
Bind(Task, File.str(), std::move(CB)));
- return TH;
+ return std::move(Cancelable.second);
}
void ClangdServer::signatureHelp(PathRef File, Position Pos,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
