This revision was automatically updated to reflect the committed changes.
Closed by commit rGc627b120eb8b: [clangd] Cancel certain operations if the file
changes before we start. (authored by sammccall).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75602/new/
https://reviews.llvm.org/D75602
Files:
clang-tools-extra/clangd/Cancellation.cpp
clang-tools-extra/clangd/Cancellation.h
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(bool(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(bool(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(bool(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(bool(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,29 @@
/// 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).
+ /// Useful for requests that are generated by clients, without any explicit
+ /// user action. These can otherwise e.g. force every version to be built.
+ 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 +249,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,26 @@
{
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), UpdateType, Invalidation,
+ std::move(Invalidate)});
}
RequestsCV.notify_all();
}
@@ -942,7 +967,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 +976,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 +1093,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 appropriate.
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
@@ -437,7 +437,8 @@
CB(std::move(Res));
};
- WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action));
+ WorkScheduler.runWithAST("EnumerateTweaks", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
@@ -556,7 +557,8 @@
CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
};
- WorkScheduler.runWithAST("Highlights", File, std::move(Action));
+ WorkScheduler.runWithAST("Highlights", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
void ClangdServer::findHover(PathRef File, Position Pos,
@@ -570,7 +572,8 @@
CB(clangd::getHover(InpAST->AST, Pos, std::move(Style), Index));
};
- WorkScheduler.runWithAST("Hover", File, std::move(Action));
+ WorkScheduler.runWithAST("Hover", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
void ClangdServer::typeHierarchy(PathRef File, Position Pos, int Resolve,
@@ -618,7 +621,8 @@
return CB(InpAST.takeError());
CB(clangd::getDocumentSymbols(InpAST->AST));
};
- WorkScheduler.runWithAST("documentSymbols", File, std::move(Action));
+ WorkScheduler.runWithAST("documentSymbols", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
void ClangdServer::findReferences(PathRef File, Position Pos, uint32_t Limit,
@@ -664,7 +668,8 @@
return CB(InpAST.takeError());
CB(clangd::getDocumentLinks(InpAST->AST));
};
- WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action));
+ WorkScheduler.runWithAST("DocumentLinks", File, std::move(Action),
+ TUScheduler::InvalidateOnUpdate);
}
std::vector<std::pair<Path, std::size_t>>
Index: clang-tools-extra/clangd/Cancellation.h
===================================================================
--- clang-tools-extra/clangd/Cancellation.h
+++ clang-tools-extra/clangd/Cancellation.h
@@ -76,6 +76,7 @@
std::pair<Context, Canceler> cancelableTask();
/// True if the current context is within a cancelable task which was cancelled.
+/// (If the context is within multiple nested tasks, true if any are 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(const Context &Ctx = Context::current());
Index: clang-tools-extra/clangd/Cancellation.cpp
===================================================================
--- clang-tools-extra/clangd/Cancellation.cpp
+++ clang-tools-extra/clangd/Cancellation.cpp
@@ -13,20 +13,30 @@
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;
- return false; // Not in scope of a task.
+ for (const CancelState *State = Ctx.get(StateKey); State != nullptr;
+ State = State->Parent)
+ if (State->Cancelled->load())
+ return true;
+ return false;
}
} // namespace clangd
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
