[clangd] Fix crash if pending computations were active on exit

[ilya-biryukov]

Summary

Make sure JSONRPCDispatcher outlives the worker threads, they access
its fields to remove the stored cancellations when Context dies.

[ioeric]

clangd/ClangdLSPServer.cpp

483 ↗

(On Diff #166684)

This woudn't work if run() is called multiple times. Maybe create a Server in each run()?

[ilya-biryukov]

clangd/ClangdLSPServer.cpp

483 ↗

(On Diff #166684)

run() asserts it's never called multiple times, so we don't break the contract.

[ioeric]

lgtm

clang-tools-extra/trunk/clangd/ClangdLSPServer.h

 #include "DraftStore.h"
 #include "FindSymbols.h"
 #include "GlobalCompilationDatabase.h"
 #include "Path.h"
 #include "Protocol.h"
 #include "ProtocolHandlers.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
+#include <memory>
 
 namespace clang {
 namespace clangd {
 
 class JSONOutput;
 class SymbolIndex;
 
 /// This class exposes ClangdServer's capabilities via Language Server Protocol.
private:
   SymbolKindBitset SupportedSymbolKinds;
 
   // Store of the current versions of the open documents.
   DraftStore DraftMgr;
 
   // Server must be the last member of the class to allow its destructor to exit
   // the worker thread that may otherwise run an async callback on partially
   // destructed instance of ClangdLSPServer.
-  ClangdServer Server;
+  // Set in construtor and destroyed when run() finishes. To ensure all worker
+  // threads exit before run() returns.
+  std::unique_ptr<ClangdServer> Server;
 };
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDLSPSERVER_H

clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp

 
 } // namespace
 
 void ClangdLSPServer::onInitialize(InitializeParams &Params) {
   if (Params.initializationOptions)
     applyConfiguration(*Params.initializationOptions);
 
   if (Params.rootUri && *Params.rootUri)
-    Server.setRootPath(Params.rootUri->file());
+    Server->setRootPath(Params.rootUri->file());
   else if (Params.rootPath && !Params.rootPath->empty())
-    Server.setRootPath(*Params.rootPath);
+    Server->setRootPath(*Params.rootPath);
 
   CCOpts.EnableSnippets =
       Params.capabilities.textDocument.completion.completionItem.snippetSupport;
   DiagOpts.EmbedFixesInDiagnostics =
       Params.capabilities.textDocument.publishDiagnostics.clangdFixSupport;
   DiagOpts.SendDiagnosticCategory =
       Params.capabilities.textDocument.publishDiagnostics.categorySupport;
 

 void ClangdLSPServer::onDocumentDidOpen(DidOpenTextDocumentParams &Params) {
   PathRef File = Params.textDocument.uri.file();
   if (Params.metadata && !Params.metadata->extraFlags.empty())
     CDB.setExtraFlagsForFile(File, std::move(Params.metadata->extraFlags));
 
   std::string &Contents = Params.textDocument.text;
 
   DraftMgr.addDraft(File, Contents);
-  Server.addDocument(File, Contents, WantDiagnostics::Yes);
+  Server->addDocument(File, Contents, WantDiagnostics::Yes);
 }
 
 void ClangdLSPServer::onDocumentDidChange(DidChangeTextDocumentParams &Params) {
   auto WantDiags = WantDiagnostics::Auto;
   if (Params.wantDiagnostics.hasValue())
     WantDiags = Params.wantDiagnostics.getValue() ? WantDiagnostics::Yes
                                                   : WantDiagnostics::No;
 
   PathRef File = Params.textDocument.uri.file();
   llvm::Expected<std::string> Contents =
       DraftMgr.updateDraft(File, Params.contentChanges);
   if (!Contents) {
     // If this fails, we are most likely going to be not in sync anymore with
     // the client.  It is better to remove the draft and let further operations
     // fail rather than giving wrong results.
     DraftMgr.removeDraft(File);
-    Server.removeDocument(File);
+    Server->removeDocument(File);
     CDB.invalidate(File);
     elog("Failed to update {0}: {1}", File, Contents.takeError());
     return;
   }
 
-  Server.addDocument(File, *Contents, WantDiags);
+  Server->addDocument(File, *Contents, WantDiags);
 }
 
 void ClangdLSPServer::onFileEvent(DidChangeWatchedFilesParams &Params) {
-  Server.onFileEvent(Params);
+  Server->onFileEvent(Params);
 }
 
 void ClangdLSPServer::onCommand(ExecuteCommandParams &Params) {
   auto ApplyEdit = [](WorkspaceEdit WE) {
     ApplyWorkspaceEditParams Edit;
     Edit.edit = std::move(WE);
     // We don't need the response so id == 1 is OK.
     // Ideally, we would wait for the response and if there is no error, we
if (Params.command == ExecuteCommandParams::CLANGD_APPLY_FIX_COMMAND &&
     // more commands are added, this will be here has a safe guard.
     replyError(
         ErrorCode::InvalidParams,
         llvm::formatv("Unsupported command \"{0}\".", Params.command).str());
   }
 }
 
 void ClangdLSPServer::onWorkspaceSymbol(WorkspaceSymbolParams &Params) {
-  Server.workspaceSymbols(
+  Server->workspaceSymbols(
       Params.query, CCOpts.Limit,
       [this](llvm::Expected<std::vector<SymbolInformation>> Items) {
         if (!Items)
           return replyError(ErrorCode::InternalError,
                             llvm::toString(Items.takeError()));
         for (auto &Sym : *Items)
           Sym.kind = adjustKindToCapability(Sym.kind, SupportedSymbolKinds);
 
         reply(json::Array(*Items));
       });
 }
 
 void ClangdLSPServer::onRename(RenameParams &Params) {
   Path File = Params.textDocument.uri.file();
   llvm::Optional<std::string> Code = DraftMgr.getDraft(File);
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onRename called for non-added file");
 
-  Server.rename(
+  Server->rename(
       File, Params.position, Params.newName,
       [File, Code,
        Params](llvm::Expected<std::vector<tooling::Replacement>> Replacements) {
         if (!Replacements)
           return replyError(ErrorCode::InternalError,
                             llvm::toString(Replacements.takeError()));
 
         // Turn the replacements into the format specified by the Language
         // Server Protocol. Fuse them into one big JSON array.
         std::vector<TextEdit> Edits;
         for (const auto &R : *Replacements)
           Edits.push_back(replacementToEdit(*Code, R));
         WorkspaceEdit WE;
         WE.changes = {{Params.textDocument.uri.uri(), Edits}};
         reply(WE);
       });
 }
 
 void ClangdLSPServer::onDocumentDidClose(DidCloseTextDocumentParams &Params) {
   PathRef File = Params.textDocument.uri.file();
   DraftMgr.removeDraft(File);
-  Server.removeDocument(File);
+  Server->removeDocument(File);
   CDB.invalidate(File);
 }
 
 void ClangdLSPServer::onDocumentOnTypeFormatting(
     DocumentOnTypeFormattingParams &Params) {
   auto File = Params.textDocument.uri.file();
   auto Code = DraftMgr.getDraft(File);
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onDocumentOnTypeFormatting called for non-added file");
 
-  auto ReplacementsOrError = Server.formatOnType(*Code, File, Params.position);
+  auto ReplacementsOrError = Server->formatOnType(*Code, File, Params.position);
   if (ReplacementsOrError)
     reply(json::Array(replacementsToEdits(*Code, ReplacementsOrError.get())));
   else
     replyError(ErrorCode::UnknownErrorCode,
                llvm::toString(ReplacementsOrError.takeError()));
 }
 
 void ClangdLSPServer::onDocumentRangeFormatting(
     DocumentRangeFormattingParams &Params) {
   auto File = Params.textDocument.uri.file();
   auto Code = DraftMgr.getDraft(File);
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onDocumentRangeFormatting called for non-added file");
 
-  auto ReplacementsOrError = Server.formatRange(*Code, File, Params.range);
+  auto ReplacementsOrError = Server->formatRange(*Code, File, Params.range);
   if (ReplacementsOrError)
     reply(json::Array(replacementsToEdits(*Code, ReplacementsOrError.get())));
   else
     replyError(ErrorCode::UnknownErrorCode,
                llvm::toString(ReplacementsOrError.takeError()));
 }
 
 void ClangdLSPServer::onDocumentFormatting(DocumentFormattingParams &Params) {
   auto File = Params.textDocument.uri.file();
   auto Code = DraftMgr.getDraft(File);
   if (!Code)
     return replyError(ErrorCode::InvalidParams,
                       "onDocumentFormatting called for non-added file");
 
-  auto ReplacementsOrError = Server.formatFile(*Code, File);
+  auto ReplacementsOrError = Server->formatFile(*Code, File);
   if (ReplacementsOrError)
     reply(json::Array(replacementsToEdits(*Code, ReplacementsOrError.get())));
   else
     replyError(ErrorCode::UnknownErrorCode,
                llvm::toString(ReplacementsOrError.takeError()));
 }
 
 void ClangdLSPServer::onDocumentSymbol(DocumentSymbolParams &Params) {
-  Server.documentSymbols(
+  Server->documentSymbols(
       Params.textDocument.uri.file(),
       [this](llvm::Expected<std::vector<SymbolInformation>> Items) {
         if (!Items)
           return replyError(ErrorCode::InvalidParams,
                             llvm::toString(Items.takeError()));
         for (auto &Sym : *Items)
           Sym.kind = adjustKindToCapability(Sym.kind, SupportedSymbolKinds);
         reply(json::Array(*Items));
for (auto &F : getFixes(Params.textDocument.uri.file(), D)) {
           {"arguments", {WE}},
       });
     }
   }
   reply(std::move(Commands));
 }
 
 void ClangdLSPServer::onCompletion(TextDocumentPositionParams &Params) {
-  Server.codeComplete(Params.textDocument.uri.file(), Params.position, CCOpts,
+  Server->codeComplete(Params.textDocument.uri.file(), Params.position, CCOpts,
                        [this](llvm::Expected<CodeCompleteResult> List) {
                          if (!List)
                            return replyError(List.takeError());
                          CompletionList LSPList;
                          LSPList.isIncomplete = List->HasMore;
                          for (const auto &R : List->Completions)
                            LSPList.items.push_back(R.render(CCOpts));
                          return reply(std::move(LSPList));
                        });
 }
 
 void ClangdLSPServer::onSignatureHelp(TextDocumentPositionParams &Params) {
-  Server.signatureHelp(Params.textDocument.uri.file(), Params.position,
+  Server->signatureHelp(Params.textDocument.uri.file(), Params.position,
                         [](llvm::Expected<SignatureHelp> SignatureHelp) {
                           if (!SignatureHelp)
                             return replyError(
                                 ErrorCode::InvalidParams,
                                 llvm::toString(SignatureHelp.takeError()));
                           reply(*SignatureHelp);
                         });
 }
 
 void ClangdLSPServer::onGoToDefinition(TextDocumentPositionParams &Params) {
-  Server.findDefinitions(Params.textDocument.uri.file(), Params.position,
+  Server->findDefinitions(Params.textDocument.uri.file(), Params.position,
                           [](llvm::Expected<std::vector<Location>> Items) {
                             if (!Items)
                               return replyError(
                                   ErrorCode::InvalidParams,
                                   llvm::toString(Items.takeError()));
                             reply(json::Array(*Items));
                           });
 }
 
 void ClangdLSPServer::onSwitchSourceHeader(TextDocumentIdentifier &Params) {
-  llvm::Optional<Path> Result = Server.switchSourceHeader(Params.uri.file());
+  llvm::Optional<Path> Result = Server->switchSourceHeader(Params.uri.file());
   reply(Result ? URI::createFile(*Result).toString() : "");
 }
 
 void ClangdLSPServer::onDocumentHighlight(TextDocumentPositionParams &Params) {
-  Server.findDocumentHighlights(
+  Server->findDocumentHighlights(
       Params.textDocument.uri.file(), Params.position,
       [](llvm::Expected<std::vector<DocumentHighlight>> Highlights) {
         if (!Highlights)
           return replyError(ErrorCode::InternalError,
                             llvm::toString(Highlights.takeError()));
         reply(json::Array(*Highlights));
       });
 }
 
 void ClangdLSPServer::onHover(TextDocumentPositionParams &Params) {
-  Server.findHover(Params.textDocument.uri.file(), Params.position,
+  Server->findHover(Params.textDocument.uri.file(), Params.position,
                     [](llvm::Expected<llvm::Optional<Hover>> H) {
                       if (!H) {
                         replyError(ErrorCode::InternalError,
                                    llvm::toString(H.takeError()));
                         return;
                       }
 
                       reply(*H);
                     });
 }
 
 void ClangdLSPServer::applyConfiguration(
     const ClangdConfigurationParamsChange &Settings) {
   // Compilation database change.
   if (Settings.compilationDatabasePath.hasValue()) {
     CDB.setCompileCommandsDir(Settings.compilationDatabasePath.getValue());
 

 
 // FIXME: This function needs to be properly tested.
 void ClangdLSPServer::onChangeConfiguration(
     DidChangeConfigurationParams &Params) {
   applyConfiguration(Params.settings);
 }
 
 void ClangdLSPServer::onReference(ReferenceParams &Params) {
-  Server.findReferences(Params.textDocument.uri.file(), Params.position,
+  Server->findReferences(Params.textDocument.uri.file(), Params.position,
                          [](llvm::Expected<std::vector<Location>> Locations) {
                            if (!Locations)
                              return replyError(
                                  ErrorCode::InternalError,
                                  llvm::toString(Locations.takeError()));
                            reply(llvm::json::Array(*Locations));
                          });
 }
 
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out,
                                  const clangd::CodeCompleteOptions &CCOpts,
                                  llvm::Optional<Path> CompileCommandsDir,
                                  bool ShouldUseInMemoryCDB,
                                  const ClangdServer::Options &Opts)
     : Out(Out), CDB(ShouldUseInMemoryCDB ? CompilationDB::makeInMemory()
                                          : CompilationDB::makeDirectoryBased(
                                                std::move(CompileCommandsDir))),
       CCOpts(CCOpts), SupportedSymbolKinds(defaultSymbolKinds()),
-      Server(CDB.getCDB(), FSProvider, /*DiagConsumer=*/*this, Opts) {}
+      Server(new ClangdServer(CDB.getCDB(), FSProvider, /*DiagConsumer=*/*this,
+                              Opts)) {}
 
 bool ClangdLSPServer::run(std::FILE *In, JSONStreamStyle InputStyle) {
   assert(!IsDone && "Run was called before");
+  assert(Server);
 
   // Set up JSONRPCDispatcher.
   JSONRPCDispatcher Dispatcher([](const json::Value &Params) {
     replyError(ErrorCode::MethodNotFound, "method not found");
   });
   registerCallbackHandlers(Dispatcher, /*Callbacks=*/*this);
 
   // Run the Language Server loop.
   runLanguageServerLoop(In, Out, InputStyle, Dispatcher, IsDone);
 
   // Make sure IsDone is set to true after this method exits to ensure assertion
   // at the start of the method fires if it's ever executed again.
   IsDone = true;
+  // Destroy ClangdServer to ensure all worker threads finish.
+  Server.reset();
 
   return ShutdownRequestReceived;
 }
 
 std::vector<Fix> ClangdLSPServer::getFixes(StringRef File,
                                            const clangd::Diagnostic &D) {
   std::lock_guard<std::mutex> Lock(FixItsMutex);
   auto DiagToFixItsIter = FixItsMap.find(File);
Out.writeMessage(json::Object{
            {"uri", URIForFile{File}},
            {"diagnostics", std::move(DiagnosticsJSON)},
        }},
   });
 }
 
 void ClangdLSPServer::reparseOpenedFiles() {
   for (const Path &FilePath : DraftMgr.getActiveFiles())
-    Server.addDocument(FilePath, *DraftMgr.getDraft(FilePath),
+    Server->addDocument(FilePath, *DraftMgr.getDraft(FilePath),
                         WantDiagnostics::Auto);
 }
 
 ClangdLSPServer::CompilationDB ClangdLSPServer::CompilationDB::makeInMemory() {
   return CompilationDB(llvm::make_unique<InMemoryCompilationDb>(), nullptr,
                        /*IsDirectoryBased=*/false);
 }
 
 ClangdLSPServer::CompilationDB