ilya-biryukov added a comment.
Sorry for missing this one. The biggest concern that I have is about the
thread-safety of the API, it's too easy to misuse from multiple threads at this
point.
We should define thread-safety guarantees for `TaskHandle` and
`CancellationToken` and make sure they're not easily broken. The suggested way
is to make both move-only types.
For `CancellationToken` we might need a non-thread-safe copy operation, though.
But maybe make it a separate method (`clone`?) with a clear statement that it's
not thread-safe?
Other than that just NITs.
================
Comment at: clangd/Cancellation.cpp:17
+namespace {
+static Key<std::shared_ptr<std::atomic<bool>>> CancellationTokenKey;
+} // namespace
----------------
Having a `shared_ptr` key in the Context can cause data races (e.g. if we copy
it concurrently from multiple threads).
I suggest we make `CancellationToken` move-only (i.e. disallow copies) and
return `const CancellationToken&` when getting it from the context.
================
Comment at: clangd/Cancellation.cpp:34
+llvm::Error CancellationHandler::getCancellationError() {
+ return llvm::make_error<TaskCancelledError>();
+}
----------------
I'm not sure if this function buys us much in terms of clarity, maybe remove it
and inline everywhere?
================
Comment at: clangd/Cancellation.h:9
+//===----------------------------------------------------------------------===//
+// CancellationToken mechanism for async threads. The caller can generate a
+// TaskHandle for cancellable tasks, then bind that handle to current context
----------------
The comments seems to mention all the important bits, but maybe we could
separate different concerns more clearly based on how the code should be used?
E.g. here's a rough draft to illustrate the idea:
```
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(function<void(llvm::Expected<ResultType>)>
Callback);
The callers of async methods (2) can issue cancellations and should be prepared
to handle `TaskCancelledError` result:
TaskHandle H = someAsyncMethod([](llvm::Expected<ResultType> R) {
if (/* code to check for TaskCancelledError */)
llvm::errs() << "Task got cancelled";
else
llvm::errs() << "The result is: " << *R;
}
});
sleep(5);
H.cancel();
The worker code itself (3) should check for cancellations using
`CancellationToken` that can be retrieved via
`CancellationToken::isCancelled()`.
void codeComplete() {
const CancellationToken& CT = CancellationToken::isCancelled();
if (CT.isCancelled())
return llvm::makeError<TaskCancelledError>();
runSema();
if (CT.isCancelled())
return llvm::makeError<TaskCancelledError>();
queryIndex();
return results;
}
If the operation was cancelled before it could run to completion, it should
propagate the TaskCancelledError as a result.
```
================
Comment at: clangd/Cancellation.h:60
+
+class CancellationToken {
+private:
----------------
We're missing docs on this and the other classes. I suggest a small doc
describing what they're used for, e.g. `used for checking if the operation was
cancelled` and `used to signal the operation should be cancelled`, etc.
Also noting the threading-safety guarantees is a probably a good idea.
================
Comment at: clangd/Cancellation.h:61
+class CancellationToken {
+private:
+ std::shared_ptr<const std::atomic<bool>> Token;
----------------
NIT: move members to the end of the class to be consistent with the rest of
clangd.
================
Comment at: clangd/Cancellation.h:67
+ operator bool() const { return isCancelled(); }
+ CancellationToken(const std::shared_ptr<const std::atomic<bool>> Token)
+ : Token(Token) {}
----------------
Can we make this ctor private?
================
Comment at: clangd/Cancellation.h:71
+
+class TaskHandle {
+public:
----------------
I wonder if we should make `TaskHandle` move-only?
The reasoning is that is can be easily used from multiple threads (since it's
used by code dealing with async tasks anyway) and copying it concurrently is a
data race.
On the other hand, calling `cancel()` is perfectly thread-safe and shouldn't
cause any problems.
================
Comment at: clangd/Cancellation.h:74
+ void cancel();
+ static TaskHandle createCancellableTaskHandle();
+ friend class CancellationHandler;
----------------
NIT: maybe use a shorter name, e.g. `TaskHandle::create`?
================
Comment at: clangd/Cancellation.h:82
+
+class CancellationHandler {
+public:
----------------
Maybe remove `CancellationHandler` and put all its functions into the clangd
namespace? It's somewhat easily confused with `CancellationToken`.
We could declare some function friends if we need to
================
Comment at: clangd/Cancellation.h:89
+
+class TaskCancelledError : public llvm::ErrorInfo<TaskCancelledError> {
+public:
----------------
NIT: maybe use a shorter name: `CancelledError`?
================
Comment at: clangd/ClangdLSPServer.cpp:355
+ Error Uncaught =
+ handleErrors(List.takeError(), [](const TaskCancelledError &) {
+ replyError(ErrorCode::RequestCancelled,
----------------
I expect the pattern `if cancelled -> reply with ErrorCode::RequestCancelled
else reply with ErrorCode::InvalidParams` to be pretty common when we add
cancellation to more operations.
Maybe add a small helper that does that? E.g.
```
/// Replies to a request with an error, passing proper error codes to the LSP
client.
/// Properly handles cancellations.
void replyError(Error E);
```
================
Comment at: clangd/ClangdLSPServer.cpp:627
+void ClangdLSPServer::onCancelRequest(CancelParams &Params) {
+ std::lock_guard<std::mutex> _(TaskHandlesMutex);
+ const auto &it = TaskHandles.find(Params.ID);
----------------
NIT: use `Lock` or `L` for consistencty with the rest of clangd. it's somewhat
easy to misread `std::lock_guard<std::mutex> _(Mut)` as
`std::lock_guard<std::mutex> (Mut)`
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D50502
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
