[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

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.

1151

Maybe just use Requests.size()?

1154–1156

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

1155

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

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

Totally agree! Thanks!

1161

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/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);