[clangd] Make sure ReplyCallbacks are destroyed before RequestCancelersMutex

[kadircet]

Summary

After rL374163, replycallbacks might have a cancellable context, which
will try to access RequestCancellers on destruction. Example failure:

libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument
src/llvm-project/out/gn/obj/clang-tools-extra/clangd/test/Output/code-action-request.test.script: line 1: 90699 Abort trap: 6           clangd -log=verbose -lit-test < src/llvm-project/clang-tools-extra/clangd/test/code-action-request.test
     90705 Done                    | FileCheck -strict-whitespace src/llvm-project/clang-tools-extra/clangd/test/code-action-request.test

[ilya-biryukov]

clang-tools-extra/clangd/ClangdLSPServer.cpp

429

Please document that this has to go after RequestCancelersMutex

[thakis]

Maybe inline the error from the bot. Right now stuff on 45.33.8.238 is very in flux.

[kadircet]

• Address comments

clang-tools-extra/clangd/ClangdLSPServer.cpp

void operator()(llvm::Expected<llvm::json::Value> Reply) {
         std::lock_guard<std::mutex> Lock(Server->TranspWriter);
         Server->Transp.reply(std::move(ID), std::move(Err));
       }
     }
   };
 
   llvm::StringMap<std::function<void(llvm::json::Value)>> Notifications;
   llvm::StringMap<std::function<void(llvm::json::Value, ReplyOnce)>> Calls;
-  // The maximum number of callbacks held in clangd.
-  //
-  // We bound the maximum size to the pending map to prevent memory leakage
-  // for cases where LSP clients don't reply for the request.
-  static constexpr int MaxReplayCallbacks = 100;
-  mutable std::mutex CallMutex;
-  int NextCallID = 0; /* GUARDED_BY(CallMutex) */
-  std::deque<std::pair</*RequestID*/ int,
-                       /*ReplyHandler*/ Callback<llvm::json::Value>>>
-      ReplyCallbacks; /* GUARDED_BY(CallMutex) */
 
   // Method calls may be cancelled by ID, so keep track of their state.
   // This needs a mutex: handlers may finish on a different thread, and that's
   // when we clean up entries in the map.
   mutable std::mutex RequestCancelersMutex;
   llvm::StringMap<std::pair<Canceler, /*Cookie*/ unsigned>> RequestCancelers;
   unsigned NextRequestCookie = 0; // To disambiguate reused IDs, see below.
   void onCancel(const llvm::json::Value &Params) {
Context cancelableRequestContext(const llvm::json::Value &ID) {
     return Task.first.derive(llvm::make_scope_exit([this, StrID, Cookie] {
       std::lock_guard<std::mutex> Lock(RequestCancelersMutex);
       auto It = RequestCancelers.find(StrID);
       if (It != RequestCancelers.end() && It->second.second == Cookie)
         RequestCancelers.erase(It);
     }));
   }
 
+  // The maximum number of callbacks held in clangd.
+  //
+  // We bound the maximum size to the pending map to prevent memory leakage
+  // for cases where LSP clients don't reply for the request.
+  // This has to go after RequestCancellers and RequestCancellersMutex since it
+  // can contain a callback that has a cancelable context.
+  static constexpr int MaxReplayCallbacks = 100;
+  mutable std::mutex CallMutex;
+  int NextCallID = 0; /* GUARDED_BY(CallMutex) */
+  std::deque<std::pair</*RequestID*/ int,
+                       /*ReplyHandler*/ Callback<llvm::json::Value>>>
+      ReplyCallbacks; /* GUARDED_BY(CallMutex) */
+
   ClangdLSPServer &Server;
 };
 constexpr int ClangdLSPServer::MessageHandler::MaxReplayCallbacks;
 
 // call(), notify(), and reply() wrap the Transport, adding logging and locking.
 void ClangdLSPServer::callRaw(StringRef Method, llvm::json::Value Params,
                               Callback<llvm::json::Value> CB) {
   auto ID = MsgHandler->bindReply(std::move(CB));