[clangd] Fix raciness in code completion tests

[ilya-biryukov]

This is still racy if there were > 1 request. I wonder if there a better way to address this?

[kadircet]

• As discussed offline, making consumeRequests take an argument for how many requests to receive before returning.

[ilya-biryukov]

It just hit me that we should probably just wait for the async request to finish before returning from completion.
We might be doing this to reduce latency a little, but I don't recall us measuring that this provides a significant performance win.

Would that work?

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

1141 ↗

(On Diff #222618)

Could we wait with timeout here (a really high one, e.g. 10 seconds) here and assert we did not hit the timeout?
The wait helper from Threading.h is a nice API to do this.

This would ensure the test never actually runs into an infinite loop in practice.

1152 ↗

(On Diff #222618)

Maybe just use Requests.size()?

1156 ↗

(On Diff #222618)

Could you add a comment that the clients have to consume exactly Num requests?

I believe anything else is a misuse:

• if there are more than Num requests coming in, we'll get a similar data race to the one we're trying to fix.
• if there are less than Num requests we'll wait for the next request indefinitely

1157 ↗

(On Diff #222618)

Could we also assert no more than Num requests were captured?
To avoid unintentional misuse of the default call.

[kadircet]

• Address comments

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

1141 ↗

(On Diff #222618)

I don't think it is that important, but sure I suppose a failing test is better than a hanging one :D

[ilya-biryukov]

LGTM

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

1141 ↗

(On Diff #222618)

Totally agree! Thanks!

1161 ↗

(On Diff #222890)

This code got me thinking whether compiler is allowed to do NRVO if the local variable is const.
Given that the object is not const, we definitely cannot call a move constructor to go from Reqs to the return value.
OTOH, if NRVO applies here, this shouldn't matter.

No need to do anything, just thinking out loud.

clang-tools-extra/trunk/clangd/unittests/CodeCompleteTests.cpp

 #include "Matchers.h"
 #include "Protocol.h"
 #include "Quality.h"
 #include "SourceCode.h"
 #include "SyncAPI.h"
 #include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
+#include "Threading.h"
 #include "index/Index.h"
 #include "index/MemIndex.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <condition_variable>
+#include <mutex>
 
 namespace clang {
 namespace clangd {
 
 namespace {
 using ::llvm::Failed;
 using ::testing::AllOf;
 using ::testing::Contains;
TEST(SignatureHelpTest, OpeningParen) {
   }
 }
 
 class IndexRequestCollector : public SymbolIndex {
 public:
   bool
   fuzzyFind(const FuzzyFindRequest &Req,
             llvm::function_ref<void(const Symbol &)> Callback) const override {
-    std::lock_guard<std::mutex> Lock(Mut);
+    std::unique_lock<std::mutex> Lock(Mut);
     Requests.push_back(Req);
+    ReceivedRequestCV.notify_one();
     return true;
   }
 
   void lookup(const LookupRequest &,
               llvm::function_ref<void(const Symbol &)>) const override {}
 
   void refs(const RefsRequest &,
             llvm::function_ref<void(const Ref &)>) const override {}
 
   void relations(const RelationsRequest &,
                  llvm::function_ref<void(const SymbolID &, const Symbol &)>)
       const override {}
 
   // This is incorrect, but IndexRequestCollector is not an actual index and it
   // isn't used in production code.
   size_t estimateMemoryUsage() const override { return 0; }
 
-  const std::vector<FuzzyFindRequest> consumeRequests() const {
-    std::lock_guard<std::mutex> Lock(Mut);
+  const std::vector<FuzzyFindRequest> consumeRequests(size_t Num) const {
+    std::unique_lock<std::mutex> Lock(Mut);
+    EXPECT_TRUE(wait(Lock, ReceivedRequestCV, timeoutSeconds(10),
+                     [this, Num] { return Requests.size() == Num; }));
     auto Reqs = std::move(Requests);
     Requests = {};
     return Reqs;
   }
 
 private:
   // We need a mutex to handle async fuzzy find requests.
+  mutable std::condition_variable ReceivedRequestCV;
   mutable std::mutex Mut;
   mutable std::vector<FuzzyFindRequest> Requests;
 };
 
-std::vector<FuzzyFindRequest> captureIndexRequests(llvm::StringRef Code) {
+// Clients have to consume exactly Num requests.
+std::vector<FuzzyFindRequest> captureIndexRequests(llvm::StringRef Code,
+                                                   size_t Num = 1) {
   clangd::CodeCompleteOptions Opts;
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   completions(Code, {}, Opts);
-  return Requests.consumeRequests();
+  const auto Reqs = Requests.consumeRequests(Num);
+  EXPECT_EQ(Reqs.size(), Num);
+  return Reqs;
 }
 
 TEST(CompletionTest, UnqualifiedIdQuery) {
   auto Requests = captureIndexRequests(R"cpp(
       namespace std {}
       using namespace std;
       namespace ns {
       void f() {
TEST(CompletionTest, EnableSpeculativeIndexRequest) {
   clangd::CodeCompleteOptions Opts = {};
 
   IndexRequestCollector Requests;
   Opts.Index = &Requests;
   Opts.SpeculativeIndexRequest = true;
 
   auto CompleteAtPoint = [&](StringRef P) {
     cantFail(runCodeComplete(Server, File, Test.point(P), Opts));
-    // Sleep for a while to make sure asynchronous call (if applicable) is also
-    // triggered before callback is invoked.
-    std::this_thread::sleep_for(std::chrono::milliseconds(100));
   };
 
   CompleteAtPoint("1");
-  auto Reqs1 = Requests.consumeRequests();
+  auto Reqs1 = Requests.consumeRequests(1);
   ASSERT_EQ(Reqs1.size(), 1u);
   EXPECT_THAT(Reqs1[0].Scopes, UnorderedElementsAre("ns1::"));
 
   CompleteAtPoint("2");
-  auto Reqs2 = Requests.consumeRequests();
+  auto Reqs2 = Requests.consumeRequests(1);
   // Speculation succeeded. Used speculative index result.
   ASSERT_EQ(Reqs2.size(), 1u);
   EXPECT_EQ(Reqs2[0], Reqs1[0]);
 
   CompleteAtPoint("3");
   // Speculation failed. Sent speculative index request and the new index
   // request after sema.
-  auto Reqs3 = Requests.consumeRequests();
+  auto Reqs3 = Requests.consumeRequests(2);
   ASSERT_EQ(Reqs3.size(), 2u);
 }
 
 TEST(CompletionTest, InsertTheMostPopularHeader) {
   std::string DeclFile = URI::create(testPath("foo")).toString();
   Symbol sym = func("Func");
   sym.CanonicalDeclaration.FileURI = DeclFile.c_str();
   sym.IncludeHeaders.emplace_back("\"foo.h\"", 2);