ilya-biryukov added a comment.
Mostly LSP-specific comments and comments about making TaskHandle thread-safe.
I'm also starting to wonder if the scope of this patch is too big, we could
potentially split it into three separate bits:
1. Generic cancellation API, i.e. `Cancellation.h` and unit-tests of the API,
2. ClangdServer using the cancellation API.
3. LSP-specific bits, i.e. `cancelRequest` etc.
Not sure if it's worth splitting the last two, but splitting out the
Cancellation API in a separate change could make review simpler.
But also happy to review the whole thing and land after it's done, feel free to
pick whatever suits you best.
================
Comment at: clangd/Cancellation.cpp:17
+namespace {
+static Key<std::shared_ptr<std::atomic<bool>>> CancellationTokenKey;
+} // namespace
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > 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.
> As talked offline, copying std::shared_ptr is thread safe.
My mistake, thanks for explaining this one to me.
Copying of `CancellationToken`s LG!
================
Comment at: clangd/Cancellation.h:86
+/// Enables async tasks to check for cancellation signal, contains a read only
+/// token cached from context. Can be created with clangd::isCancelled. Totally
+/// thread-safe multiple threads can check for cancellation on the same token.
----------------
NIT: Maybe replace 'Totally thread-safe' with just `Thread-safe`? The latter
conveys the same information.
================
Comment at: clangd/Cancellation.h:90
+public:
+ bool isCancelled() const { return Token ? static_cast<bool>(*Token) : false;
}
+ operator bool() const { return isCancelled(); }
----------------
NIT: maybe use `Token->load()` instead of `static_cast<bool>(Token)`? Arguably,
looks more concise.
================
Comment at: clangd/Cancellation.h:92
+ operator bool() const { return isCancelled(); }
+ friend CancellationToken isCancelled();
+
----------------
It's a bit confusing that this name clashes with a member function.
We seem to optimize for making this work anywhere:
```
if (isCancelled()) // <-- get token from ctx, then use implicit conversion
to check if it was cancelled
return llvm::makeError<CancelledError>();
```
Which is probably a nice pattern. But we could have two functions that do the
same thing with less magic (i.e. implicit conversions) involved at the call
sites:
```
CancellationToken getCurrentCancellation();
void setCurrentCancellation(CancellationToken CT);
inline bool isCancelled() { return getCurrentCancellation().isCancelled(); }
```
Would allow to get rid of implicit conversion and friend function with the same
signature as member. Would also make cases where we actually want to stash the
token somewhere cleaner, e.g. instead of `CancellationToken CT =
isCancelled()`, we'll have `CancellationToken CT = getCurrentCancellation()`.
WDYT?
================
Comment at: clangd/Cancellation.h:101
+
+/// Enables signalling a cancellation on an asyn task. Totally thread-safe, can
+/// trigger cancellation from multiple threads or make copies. Since contains a
----------------
s/asyn/async
================
Comment at: clangd/Cancellation.h:104
+/// std::shared_ptr inside copying is costy, avoid it as much as possible.
+class TaskHandle {
+public:
----------------
As discussed offline, maybe we should make `TaskHandle` move-only?
Conceptually, `TaskHandle` is an abstraction that represent some spawned async
task and in that sense somewhat similar to futures.
================
Comment at: clangd/Cancellation.h:108
+ static TaskHandle create();
+ friend Context setCurrentCancellationToken(TaskHandle TH);
+
----------------
I wonder if we should have a `createCancellationToken()` method and a method to
stash it in the context instead?
Would help if we want to make `TaskHandle` move-only and in general seems like
a simpler user API, because things you put and get back out of the context is
the same, i.e. it's a `CancellationToken`.
WDYT?
================
Comment at: clangd/ClangdLSPServer.cpp:74
+const std::string NormalizeRequestID(const json::Value &ID) {
+ std::string NormalizedID;
----------------
NIT: return `std::string`, const does not buys us anything here.
================
Comment at: clangd/ClangdLSPServer.cpp:354
+ if (!List) {
+ return replyError(List.takeError());
+ }
----------------
NIT: remove braces around short, single-statement branches
================
Comment at: clangd/ClangdLSPServer.cpp:631
+ return;
+ const std::string &NormalizedID = NormalizeRequestID(*ID);
+ {
----------------
Use a value type here, i.e. `std::string` instead of `const std::string&`.
================
Comment at: clangd/ClangdLSPServer.cpp:642
+ return;
+ const std::string &NormalizedID = NormalizeRequestID(*ID);
+ {
----------------
Use a value type here, i.e. std::string instead of const std::string&.
================
Comment at: clangd/ClangdLSPServer.cpp:643
+ const std::string &NormalizedID = NormalizeRequestID(*ID);
+ {
+ std::lock_guard<std::mutex> Lock(TaskHandlesMutex);
----------------
NIT: remove these braces, they don't buy us anything here?
================
Comment at: clangd/ClangdLSPServer.h:178
+ // Following two functions are context-aware, they create and delete tokens
+ // associated with only their thread.
+ void CleanupTaskHandle();
----------------
I'm not sure what `associated with only their thread` means.
The tokens are associated with the request-ids rather than threads, so having
something like `..., they create and remove cancellation handles for the
request-id stored in the current Context`
================
Comment at: clangd/ClangdLSPServer.h:179
+ // associated with only their thread.
+ void CleanupTaskHandle();
+ void StoreTaskHandle(TaskHandle TH);
----------------
These two methods look like a nice suit for better wrapped in a RAII-object. It
would add the value to the associated map on construction and remove on
destruction.
One problem is that all operations calls would become weird, because we'll have
to move this RAII object around and moving into lambdas is not fun without
C++14 (which allows `[x=std::move(y)]() {}`..
Maybe add a comment that calls to this function should always be paired and a
FIXME that we should have an API that enforces this?
================
Comment at: clangd/ClangdServer.cpp:167
+ if (isCancelled()) {
+ return CB(llvm::make_error<CancelledError>());
+ }
----------------
NIT: remove braces around single-statement branches
================
Comment at: clangd/JSONRPCDispatcher.cpp:135
+ Error Err = handleErrors(std::move(E), [](const CancelledError &TCE) {
+ replyError(ErrorCode::RequestCancelled, TCE.message());
+ });
----------------
Isn't there a form of `handleErrors` that can handle the second case in the
last argument?
I.e. can we merge the following `if` into the `handleErrors` call?
================
Comment at: clangd/JSONRPCDispatcher.h:67
void replyError(ErrorCode Code, const llvm::StringRef &Message);
+void replyError(llvm::Error E);
/// Sends a request to the client.
----------------
Maybe adds docs explaining what this function does?
Specifically, what `ErrorCode` it chooses and how the message would look like
================
Comment at: clangd/JSONRPCDispatcher.h:115
+const llvm::json::Value *GetRequestId();
} // namespace clangd
----------------
naming-nit: use `lowerCamelCase` for function names
================
Comment at: clangd/JSONRPCDispatcher.h:115
+const llvm::json::Value *GetRequestId();
} // namespace clangd
----------------
ilya-biryukov wrote:
> naming-nit: use `lowerCamelCase` for function names
I suggest we document this function and put to next to `reply` and
`replyError`, since they are closely related (i.e. the latter put the current
request-id into the replies).
We should probably discourage using this function in its documentation unless
absolutely necessary, the users should probably directly call `reply` or
`replyError` instead.
================
Comment at: unittests/clangd/CancellationTests.cpp:64
+}
+
+} // namespace
----------------
Another interesting case to test:
create multiple threads, propagate cancellation into them and check they get
properly cancelled.
Using `Notification` from `Threading.h` should allow to write it in a pretty
simplistic manner.
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
