diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp --- a/clang-tools-extra/clangd/TUScheduler.cpp +++ b/clang-tools-extra/clangd/TUScheduler.cpp @@ -274,8 +274,9 @@ /// 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 Requests; /* GUARDED_BY(Mutex) */ + bool Done; /* GUARDED_BY(Mutex) */ + std::deque Requests; /* GUARDED_BY(Mutex) */ + llvm::Optional 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 @@ #ifndef NDEBUG std::lock_guard 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 @@ 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::run() { while (true) { - Request Req; { std::unique_lock 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 @@ 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( Wait.time() - steady_clock::now()) @@ -744,26 +748,28 @@ 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 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 Lock(Mutex); - Requests.pop_front(); + CurrentRequest.reset(); IsEmpty = Requests.empty(); } if (IsEmpty) @@ -843,7 +849,8 @@ bool ASTWorker::blockUntilIdle(Deadline Timeout) const { std::unique_lock 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 --- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp +++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp @@ -367,6 +367,30 @@ << "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 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());