Author: Kadir Cetinkaya Date: 2020-03-10T18:25:35+01:00 New Revision: 39eebe68b5990273a69ed527e827753e7d4dba75
URL: https://github.com/llvm/llvm-project/commit/39eebe68b5990273a69ed527e827753e7d4dba75 DIFF: https://github.com/llvm/llvm-project/commit/39eebe68b5990273a69ed527e827753e7d4dba75.diff LOG: [clangd] Use a separate RunningTask flag instead of leaving a broken request on top of the queue Summary: This helps us prevent races when scheduler (or any other thread) tries to read a request while it's still running. Reviewers: sammccall Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D75927 Added: Modified: clang-tools-extra/clangd/TUScheduler.cpp clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp index 3ce0a5b10d95..7188d0be69ff 100644 --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -274,8 +274,9 @@ class ASTWorker { /// Becomes ready when the first preamble build finishes. Notification PreambleWasBuilt; /// Set to true to signal run() to finish processing. - bool Done; /* GUARDED_BY(Mutex) */ - std::deque<Request> Requests; /* GUARDED_BY(Mutex) */ + bool Done; /* GUARDED_BY(Mutex) */ + std::deque<Request> Requests; /* GUARDED_BY(Mutex) */ + llvm::Optional<Request> CurrentRequest; /* GUARDED_BY(Mutex) */ mutable std::condition_variable RequestsCV; /// Guards the callback that publishes results of AST-related computations /// (diagnostics, highlightings) and file statuses. @@ -370,7 +371,8 @@ ASTWorker::~ASTWorker() { #ifndef NDEBUG std::lock_guard<std::mutex> Lock(Mutex); assert(Done && "handle was not destroyed"); - assert(Requests.empty() && "unprocessed requests when destroying ASTWorker"); + assert(Requests.empty() && !CurrentRequest && + "unprocessed requests when destroying ASTWorker"); #endif } @@ -606,8 +608,10 @@ void ASTWorker::getCurrentPreamble( auto LastUpdate = std::find_if(Requests.rbegin(), Requests.rend(), [](const Request &R) { return R.UpdateType.hasValue(); }); - // If there were no writes in the queue, the preamble is ready now. - if (LastUpdate == Requests.rend()) { + // If there were no writes in the queue, and CurrentRequest is not a write, + // the preamble is ready now. + if (LastUpdate == Requests.rend() && + (!CurrentRequest || CurrentRequest->UpdateType.hasValue())) { Lock.unlock(); return Callback(getPossiblyStalePreamble()); } @@ -714,9 +718,9 @@ void ASTWorker::emitTUStatus(TUAction Action, void ASTWorker::run() { while (true) { - Request Req; { std::unique_lock<std::mutex> Lock(Mutex); + assert(!CurrentRequest && "A task is already running, multiple workers?"); for (auto Wait = scheduleLocked(); !Wait.expired(); Wait = scheduleLocked()) { if (Done) { @@ -734,7 +738,7 @@ void ASTWorker::run() { Tracer.emplace("Debounce"); SPAN_ATTACH(*Tracer, "next_request", Requests.front().Name); if (!(Wait == Deadline::infinity())) { - emitTUStatus({TUAction::Queued, Req.Name}); + emitTUStatus({TUAction::Queued, Requests.front().Name}); SPAN_ATTACH(*Tracer, "sleep_ms", std::chrono::duration_cast<std::chrono::milliseconds>( Wait.time() - steady_clock::now()) @@ -744,26 +748,28 @@ void ASTWorker::run() { wait(Lock, RequestsCV, Wait); } - Req = std::move(Requests.front()); - // Leave it on the queue for now, so waiters don't see an empty queue. + CurrentRequest = std::move(Requests.front()); + Requests.pop_front(); } // unlock Mutex + // It is safe to perform reads to CurrentRequest without holding the lock as + // only writer is also this thread. { std::unique_lock<Semaphore> Lock(Barrier, std::try_to_lock); if (!Lock.owns_lock()) { - emitTUStatus({TUAction::Queued, Req.Name}); + emitTUStatus({TUAction::Queued, CurrentRequest->Name}); Lock.lock(); } - WithContext Guard(std::move(Req.Ctx)); - trace::Span Tracer(Req.Name); - emitTUStatus({TUAction::RunningAction, Req.Name}); - Req.Action(); + WithContext Guard(std::move(CurrentRequest->Ctx)); + trace::Span Tracer(CurrentRequest->Name); + emitTUStatus({TUAction::RunningAction, CurrentRequest->Name}); + CurrentRequest->Action(); } bool IsEmpty = false; { std::lock_guard<std::mutex> Lock(Mutex); - Requests.pop_front(); + CurrentRequest.reset(); IsEmpty = Requests.empty(); } if (IsEmpty) @@ -843,7 +849,8 @@ bool ASTWorker::shouldSkipHeadLocked() const { bool ASTWorker::blockUntilIdle(Deadline Timeout) const { std::unique_lock<std::mutex> Lock(Mutex); - return wait(Lock, RequestsCV, Timeout, [&] { return Requests.empty(); }); + return wait(Lock, RequestsCV, Timeout, + [&] { return Requests.empty() && !CurrentRequest; }); } // Render a TUAction to a user-facing string representation. diff --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp index c9569d2d96cc..3d6812496f47 100644 --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -367,6 +367,30 @@ TEST_F(TUSchedulerTests, Cancellation) { << "All reads other than R2B were cancelled"; } +TEST_F(TUSchedulerTests, InvalidationNoCrash) { + auto Path = testPath("foo.cpp"); + TUScheduler S(CDB, optsForTest(), captureDiags()); + + Notification StartedRunning; + Notification ScheduledChange; + // We expect invalidation logic to not crash by trying to invalidate a running + // request. + S.update(Path, getInputs(Path, ""), WantDiagnostics::Auto); + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); + S.runWithAST( + "invalidatable-but-running", Path, + [&](llvm::Expected<InputsAndAST> AST) { + StartedRunning.notify(); + ScheduledChange.wait(); + ASSERT_TRUE(bool(AST)); + }, + TUScheduler::InvalidateOnUpdate); + StartedRunning.wait(); + S.update(Path, getInputs(Path, ""), WantDiagnostics::Auto); + ScheduledChange.notify(); + ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10))); +} + TEST_F(TUSchedulerTests, Invalidation) { auto Path = testPath("foo.cpp"); TUScheduler S(CDB, optsForTest(), captureDiags()); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
