Fix possible race when setting diagnostics.

src/clang_complete.h

@@ -13,8 +13,6 @@
 #include <mutex>
 #include <string>
 
-// TODO: rename this file to clang_completion.h/cc
-
 struct CompletionSession {
   Project::Entry file;
   WorkingFiles* working_files;

src/command_line.cc

@@ -1250,16 +1250,10 @@ void EmitDiagnostics(WorkingFiles* working_files, std::string path, NonElidedVec
   IpcManager::instance()->SendOutMessageToClient(IpcId::TextDocumentPublishDiagnostics, diagnostic_response);
 
   // Cache diagnostics so we can show fixits.
-  // TODO/FIXME: this function can run concurrently on any thread!!!!
+  working_files->DoActionOnFile(path, [&](WorkingFile* working_file) {
-  // TODO/FIXME: this function can run concurrently on any thread!!!!
+    if (working_file)
-  // TODO/FIXME: this function can run concurrently on any thread!!!!
+      working_file->diagnostics_ = diagnostics;
-  // TODO/FIXME: this function can run concurrently on any thread!!!!
+  });
-  // TODO/FIXME: this function can run concurrently on any thread!!!!
-  // TODO/FIXME: this function can run concurrently on any thread!!!!
-  // TODO/FIXME: this function can run concurrently on any thread!!!!
-  WorkingFile* working_file = working_files->GetFileByFilename(path);
-  if (working_file)
-    working_file->diagnostics = diagnostics;
 }
 
 
@@ -2871,8 +2865,11 @@ bool QueryDbMainLoop(
             break;
         }
 
-
+        std::vector<lsDiagnostic> diagnostics;
-        for (lsDiagnostic& diag : working_file->diagnostics) {
+        working_files->DoAction([&]() {
+          diagnostics = working_file->diagnostics_;
+        });
+        for (lsDiagnostic& diag : diagnostics) {
           if (diag.range.start.line != msg->params.range.start.line)
             continue;
 

src/indexer.h

@@ -135,11 +135,6 @@ inline void Reflect(Writer& visitor, IndexFuncRef& value) {
   visitor.String(s.c_str());
 }
 
-// TODO: skip as much forward-processing as possible when |is_system_def| is
-//       set to false.
-// TODO: Either eliminate the defs created as a by-product of cross-referencing,
-//       or do not emit things we don't have definitions for.
-
 template <typename TypeId,
           typename FuncId,
           typename VarId,

src/working_files.cc

@@ -297,6 +297,20 @@ WorkingFile* WorkingFiles::GetFileByFilenameNoLock(const std::string& filename)
   return nullptr;
 }
 
+void WorkingFiles::DoAction(const std::function<void()>& action) {
+  std::lock_guard<std::mutex> lock(files_mutex);
+  action();
+}
+
+void WorkingFiles::DoActionOnFile(
+    const std::string& filename,
+    const std::function<void(WorkingFile* file)>& action) {
+
+  std::lock_guard<std::mutex> lock(files_mutex);
+  WorkingFile* file = GetFileByFilenameNoLock(filename);
+  action(file);
+}
+
 WorkingFile* WorkingFiles::OnOpen(const Ipc_TextDocumentDidOpen::Params& open) {
   std::lock_guard<std::mutex> lock(files_mutex);
 

src/working_files.h

@@ -33,7 +33,8 @@ struct WorkingFile {
   // Note: The items in the value entry are 1-based liness.
   std::unordered_map<std::string, std::vector<int>> all_buffer_lines_lookup;
   // A set of diagnostics that have been reported for this file.
-  std::vector<lsDiagnostic> diagnostics;
+  // NOTE: _ is appended because it must be accessed under the WorkingFiles lock!
+  std::vector<lsDiagnostic> diagnostics_;
 
   WorkingFile(const std::string& filename, const std::string& buffer_content);
 
@@ -77,6 +78,11 @@ struct WorkingFiles {
   WorkingFile* GetFileByFilename(const std::string& filename);
   WorkingFile* GetFileByFilenameNoLock(const std::string& filename);
 
+  // Run |action| under the lock.
+  void DoAction(const std::function<void()>& action);
+  // Run |action| on the file identified by |filename|. This executes under the lock.
+  void DoActionOnFile(const std::string& filename, const std::function<void(WorkingFile* file)>& action);
+
   WorkingFile* OnOpen(const Ipc_TextDocumentDidOpen::Params& open);
   void OnChange(const Ipc_TextDocumentDidChange::Params& change);
   void OnClose(const Ipc_TextDocumentDidClose::Params& close);