[PATCH] D68273: [clangd] Fix raciness in code completion tests

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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?



================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1141
+    ReceivedRequestCV.wait(Lock,
+                           [this, Num] { return NumRequestsReceived == Num; });
+    NumRequestsReceived = 0;
----------------
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.


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1152
   mutable std::mutex Mut;
+  mutable size_t NumRequestsReceived = 0;
   mutable std::vector<FuzzyFindRequest> Requests;
----------------
Maybe just use `Requests.size()`?


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1156
 
-std::vector<FuzzyFindRequest> captureIndexRequests(llvm::StringRef Code) {
+std::vector<FuzzyFindRequest> captureIndexRequests(llvm::StringRef Code,
+                                                   size_t Num = 1) {
----------------
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


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1157
+std::vector<FuzzyFindRequest> captureIndexRequests(llvm::StringRef Code,
+                                                   size_t Num = 1) {
   clangd::CodeCompleteOptions Opts;
----------------
Could we also assert no more than `Num` requests were captured?
To avoid unintentional misuse of the default call.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68273/new/

https://reviews.llvm.org/D68273