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.
Otherwise they can force us to build lots of snapshots that we don't need.
Particularly, try to do this for operations that are frequently
generated by editors without explicit user interaction, and where
editing the file makes the result less useful. (Code action
enumeration is a good example).
https://github.com/clangd/clangd/issues/298
This doesn't return the "right" LSP error code (ContentModified) to the client,
we need to teach the cancellation API to distinguish between different causes.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D75602
Files:
clang-tools-extra/clangd/Cancellation.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/JSONTransport.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/TUScheduler.h
clang-tools-extra/clangd/unittests/CancellationTests.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
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "Annotations.h"
+#include "Cancellation.h"
#include "Context.h"
#include "Diagnostics.h"
#include "Matchers.h"
@@ -346,6 +347,63 @@
<< "All reads other than R2B were cancelled";
}
+TEST_F(TUSchedulerTests, Invalidation) {
+ auto Path = testPath("foo.cpp");
+ TUScheduler S(CDB, optsForTest(), captureDiags());
+ std::atomic<int> Builds(0), Actions(0);
+
+ Notification Start;
+ updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector<Diag>) {
+ ++Builds;
+ Start.wait();
+ });
+ S.runWithAST(
+ "invalidatable", Path,
+ [&](llvm::Expected<InputsAndAST> AST) {
+ ++Actions;
+ EXPECT_FALSE(!!AST);
+ llvm::Error E = AST.takeError();
+ EXPECT_TRUE(E.isA<CancelledError>());
+ consumeError(std::move(E));
+ },
+ TUScheduler::InvalidateOnUpdate);
+ S.runWithAST(
+ "not-invalidatable", Path,
+ [&](llvm::Expected<InputsAndAST> AST) {
+ ++Actions;
+ EXPECT_TRUE(!!AST);
+ },
+ TUScheduler::NoInvalidation);
+ updateWithDiags(S, Path, "b", WantDiagnostics::Auto, [&](std::vector<Diag>) {
+ ++Builds;
+ ADD_FAILURE() << "Shouldn't build, all dependents invalidated";
+ });
+ S.runWithAST(
+ "invalidatable", Path,
+ [&](llvm::Expected<InputsAndAST> AST) {
+ ++Actions;
+ EXPECT_FALSE(!!AST);
+ llvm::Error E = AST.takeError();
+ EXPECT_TRUE(E.isA<CancelledError>());
+ consumeError(std::move(E));
+ },
+ TUScheduler::InvalidateOnUpdate);
+ updateWithDiags(S, Path, "c", WantDiagnostics::Auto,
+ [&](std::vector<Diag>) { ++Builds; });
+ S.runWithAST(
+ "invalidatable", Path,
+ [&](llvm::Expected<InputsAndAST> AST) {
+ ++Actions;
+ EXPECT_TRUE(!!AST) << "Shouldn't be invalidated, no update follows";
+ },
+ TUScheduler::InvalidateOnUpdate);
+ Start.notify();
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+
+ EXPECT_EQ(2, Builds.load()) << "Middle build should be skipped";
+ EXPECT_EQ(4, Actions.load()) << "All actions should run (some with error)";
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
Index: clang-tools-extra/clangd/unittests/CancellationTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CancellationTests.cpp
+++ clang-tools-extra/clangd/unittests/CancellationTests.cpp
@@ -44,6 +44,30 @@
Task.second();
}
+struct NestedTasks {
+ std::pair<Context, Canceler> Outer, Inner;
+ NestedTasks() {
+ Outer = cancelableTask();
+ {
+ WithContext WithOuter(Outer.first.clone());
+ Inner = cancelableTask();
+ }
+ }
+};
+
+TEST(CancellationTest, Nested) {
+ // Cancelling inner task works but leaves outer task unaffected.
+ NestedTasks CancelInner;
+ CancelInner.Inner.second();
+ EXPECT_TRUE(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));
+}
+
TEST(CancellationTest, AsynCancellationTest) {
std::atomic<bool> HasCancelled(false);
Notification Cancelled;
Index: clang-tools-extra/clangd/TUScheduler.h
===================================================================
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -219,6 +219,27 @@
/// Schedule an async task with no dependencies.
void run(llvm::StringRef Name, llvm::unique_function<void()> Action);
+ /// Defines how a runWithAST action is implicitly cancelled by other actions.
+ enum ASTActionInvalidation {
+ /// The request will run unless explicitly cancelled.
+ NoInvalidation,
+ /// The request will be implicitly cancelled by a subsequent update().
+ /// (Only if the request was not yet cancelled).
+ InvalidateOnUpdate,
+ };
+
+ /// Error to return when an ASTActionInvalidation policy fires.
+ class InvalidatedError : public llvm::ErrorInfo<InvalidatedError> {
+ public:
+ static char ID;
+ ASTActionInvalidation Policy;
+
+ void log(llvm::raw_ostream &OS) const override;
+ std::error_code convertToErrorCode() const override {
+ return std::make_error_code(std::errc::interrupted);
+ }
+ };
+
/// Schedule an async read of the AST. \p Action will be called when AST is
/// ready. The AST passed to \p Action refers to the version of \p File
/// tracked at the time of the call, even if new updates are received before
@@ -226,9 +247,11 @@
/// If an error occurs during processing, it is forwarded to the \p Action
/// callback.
/// If the context is cancelled before the AST is ready, the callback will
- /// receive a CancelledError.
+ /// receive a CancelledError. If the invalidation policy is triggered, the
+ /// callback will receive an InvalidatedError.
void runWithAST(llvm::StringRef Name, PathRef File,
- Callback<InputsAndAST> Action);
+ Callback<InputsAndAST> Action,
+ ASTActionInvalidation = NoInvalidation);
/// Controls whether preamble reads wait for the preamble to be up-to-date.
enum PreambleConsistency {
Index: clang-tools-extra/clangd/TUScheduler.cpp
===================================================================
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -184,7 +184,8 @@
void update(ParseInputs Inputs, WantDiagnostics);
void
runWithAST(llvm::StringRef Name,
- llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action);
+ llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
+ TUScheduler::ASTActionInvalidation);
bool blockUntilIdle(Deadline Timeout) const;
std::shared_ptr<const PreambleData> getPossiblyStalePreamble() const;
@@ -214,7 +215,8 @@
void stop();
/// Adds a new task to the end of the request queue.
void startTask(llvm::StringRef Name, llvm::unique_function<void()> Task,
- llvm::Optional<WantDiagnostics> UpdateType);
+ llvm::Optional<WantDiagnostics> UpdateType,
+ TUScheduler::ASTActionInvalidation);
/// Updates the TUStatus and emits it. Only called in the worker thread.
void emitTUStatus(TUAction FAction,
const TUStatus::BuildDetails *Detail = nullptr);
@@ -237,6 +239,8 @@
steady_clock::time_point AddTime;
Context Ctx;
llvm::Optional<WantDiagnostics> UpdateType;
+ TUScheduler::ASTActionInvalidation InvalidationPolicy;
+ Canceler Invalidate;
};
/// Handles retention of ASTs.
@@ -542,12 +546,13 @@
// Stash the AST in the cache for further use.
IdleASTs.put(this, std::move(*AST));
};
- startTask(TaskName, std::move(Task), WantDiags);
+ startTask(TaskName, std::move(Task), WantDiags, TUScheduler::NoInvalidation);
}
void ASTWorker::runWithAST(
llvm::StringRef Name,
- llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action) {
+ 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>());
@@ -576,7 +581,7 @@
"invalid AST", llvm::errc::invalid_argument));
Action(InputsAndAST{*CurrentInputs, **AST});
};
- startTask(Name, std::move(Task), /*UpdateType=*/None);
+ startTask(Name, std::move(Task), /*UpdateType=*/None, Invalidation);
}
std::shared_ptr<const PreambleData>
@@ -606,7 +611,9 @@
},
"GetPreamble", steady_clock::now(),
Context::current().clone(),
- /*UpdateType=*/None});
+ /*UpdateType=*/None,
+ /*InvalidationPolicy=*/TUScheduler::NoInvalidation,
+ /*Invalidate=*/nullptr});
Lock.unlock();
RequestsCV.notify_all();
}
@@ -650,7 +657,8 @@
void ASTWorker::startTask(llvm::StringRef Name,
llvm::unique_function<void()> Task,
- llvm::Optional<WantDiagnostics> UpdateType) {
+ llvm::Optional<WantDiagnostics> UpdateType,
+ TUScheduler::ASTActionInvalidation Invalidation) {
if (RunSync) {
assert(!Done && "running a task after stop()");
trace::Span Tracer(Name + ":" + llvm::sys::path::filename(FileName));
@@ -661,9 +669,25 @@
{
std::lock_guard<std::mutex> Lock(Mutex);
assert(!Done && "running a task after stop()");
- Requests.push_back(
- {std::move(Task), std::string(Name), steady_clock::now(),
- Context::current().derive(kFileBeingProcessed, FileName), UpdateType});
+ // Cancel any requests invalidated by this request.
+ if (UpdateType)
+ for (auto &R : llvm::reverse(Requests)) {
+ if (R.InvalidationPolicy == TUScheduler::InvalidateOnUpdate)
+ R.Invalidate();
+ if (R.UpdateType)
+ break; // Older requests were already invalidated by the older update.
+ }
+
+ // Allow this request to be cancelled if invalidated.
+ Context Ctx = Context::current().derive(kFileBeingProcessed, FileName);
+ Canceler Invalidate = nullptr;
+ if (Invalidation) {
+ WithContext WC(std::move(Ctx));
+ std::tie(Ctx, Invalidate) = cancelableTask();
+ }
+ Requests.push_back({std::move(Task), std::string(Name), steady_clock::now(),
+ std::move(Ctx).derive(kFileBeingProcessed, FileName),
+ UpdateType, Invalidation, std::move(Invalidate)});
}
RequestsCV.notify_all();
}
@@ -942,7 +966,8 @@
void TUScheduler::runWithAST(
llvm::StringRef Name, PathRef File,
- llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action) {
+ llvm::unique_function<void(llvm::Expected<InputsAndAST>)> Action,
+ TUScheduler::ASTActionInvalidation Invalidation) {
auto It = Files.find(File);
if (It == Files.end()) {
Action(llvm::make_error<LSPError>(
@@ -950,7 +975,7 @@
return;
}
- It->second->Worker->runWithAST(Name, std::move(Action));
+ It->second->Worker->runWithAST(Name, std::move(Action), Invalidation);
}
void TUScheduler::runWithPreamble(llvm::StringRef Name, PathRef File,
@@ -1067,5 +1092,15 @@
return P;
}
+void TUScheduler::InvalidatedError::log(llvm::raw_ostream &OS) const {
+ switch (Policy) {
+ case InvalidateOnUpdate:
+ OS << "Task was cancelled due to a subsequent change to the file.";
+ break;
+ case NoInvalidation:
+ llvm_unreachable("Invalidated for no reason?");
+ }
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/JSONTransport.cpp
===================================================================
--- clang-tools-extra/clangd/JSONTransport.cpp
+++ clang-tools-extra/clangd/JSONTransport.cpp
@@ -19,6 +19,8 @@
llvm::json::Object encodeError(llvm::Error E) {
std::string Message;
ErrorCode Code = ErrorCode::UnknownErrorCode;
+ // FIXME: encode cancellation errors using RequestCancelled or ContentModified
+ // as approprate.
if (llvm::Error Unhandled = llvm::handleErrors(
std::move(E), [&](const LSPError &L) -> llvm::Error {
Message = L.Message;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -53,6 +53,10 @@
namespace clang {
namespace clangd {
namespace {
+/// Invalidation policy for actions that are likely generated by the editor
+/// without any explicit user action.
+static TUScheduler::ASTActionInvalidation LikelyImplicit =
+ TUScheduler::InvalidateOnUpdate;
// Update the FileIndex with new ASTs and plumb the diagnostics responses.
struct UpdateIndexCallbacks : public ParsingCallbacks {
@@ -437,7 +441,8 @@
CB(std::move(Res));
};
- WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action));
+ WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action),
+ LikelyImplicit);
}
void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
@@ -556,7 +561,8 @@
CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
};
- WorkScheduler.runWithAST("Highlights", File, std::move(Action));
+ WorkScheduler.runWithAST("Highlights", File, std::move(Action),
+ LikelyImplicit);
}
void ClangdServer::findHover(PathRef File, Position Pos,
@@ -570,7 +576,7 @@
CB(clangd::getHover(InpAST->AST, Pos, std::move(Style), Index));
};
- WorkScheduler.runWithAST("Hover", File, std::move(Action));
+ WorkScheduler.runWithAST("Hover", File, std::move(Action), LikelyImplicit);
}
void ClangdServer::typeHierarchy(PathRef File, Position Pos, int Resolve,
@@ -618,7 +624,8 @@
return CB(InpAST.takeError());
CB(clangd::getDocumentSymbols(InpAST->AST));
};
- WorkScheduler.runWithAST("documentSymbols", File, std::move(Action));
+ WorkScheduler.runWithAST("documentSymbols", File, std::move(Action),
+ LikelyImplicit);
}
void ClangdServer::findReferences(PathRef File, Position Pos, uint32_t Limit,
@@ -664,7 +671,8 @@
return CB(InpAST.takeError());
CB(clangd::getDocumentLinks(InpAST->AST));
};
- WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action));
+ WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action),
+ LikelyImplicit);
}
std::vector<std::pair<Path, std::size_t>>
Index: clang-tools-extra/clangd/Cancellation.cpp
===================================================================
--- clang-tools-extra/clangd/Cancellation.cpp
+++ clang-tools-extra/clangd/Cancellation.cpp
@@ -13,19 +13,29 @@
namespace clangd {
char CancelledError::ID = 0;
-static Key<std::shared_ptr<std::atomic<bool>>> FlagKey;
+
+// We don't want a cancelable scope to "shadow" an enclosing one.
+struct CancelState {
+ std::shared_ptr<std::atomic<bool>> Cancelled;
+ const CancelState *Parent;
+};
+static Key<CancelState> StateKey;
std::pair<Context, Canceler> cancelableTask() {
- auto Flag = std::make_shared<std::atomic<bool>>();
+ CancelState State;
+ State.Cancelled = std::make_shared<std::atomic<bool>>();
+ State.Parent = Context::current().get(StateKey);
return {
- Context::current().derive(FlagKey, Flag),
- [Flag] { *Flag = true; },
+ Context::current().derive(StateKey, State),
+ [Flag(State.Cancelled)] { *Flag = true; },
};
}
bool isCancelled(const Context &Ctx) {
- if (auto *Flag = Ctx.get(FlagKey))
- return **Flag;
+ for (const CancelState *State = Ctx.get(StateKey); State != nullptr;
+ State = State->Parent)
+ if (State->Cancelled->load())
+ return true;
return false; // Not in scope of a task.
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
