#5977: Rearrange the signal emission and parser thread cleanup code t…

…o avoid deadlocks. Reload Decls signal emission is happening on the thread it was invoked on, otherwise we run into openGL thread access problems.

radiantcore/decl/DeclarationFolderParser.cpp

@@ -66,7 +66,7 @@ void DeclarationFolderParser::parse(std::istream& stream, const vfs::FileInfo& f
 void DeclarationFolderParser::onFinishParsing()
 {
     // Submit all parsed declarations to the decl manager
-    _owner.onParserFinished(_defaultDeclType, _parsedBlocks);
+    _owner.onParserFinished(_defaultDeclType, std::move(_parsedBlocks));
 }
 
 Type DeclarationFolderParser::determineBlockType(const DeclarationBlockSyntax& block)

radiantcore/decl/DeclarationFolderParser.h

@@ -10,6 +10,8 @@ namespace decl
 
 class DeclarationManager;
 
+using ParseResult = std::map<Type, std::vector<DeclarationBlockSyntax>>;
+
 // Threaded parser processing all files in the configured decl folder
 // Submits all parsed declarations to the IDeclarationManager when finished
 class DeclarationFolderParser :
@@ -22,7 +24,7 @@ class DeclarationFolderParser :
     std::map<std::string, Type, string::ILess> _typeMapping;
 
     // Holds all the identified blocks of all visited files
-    std::map<Type, std::vector<DeclarationBlockSyntax>> _parsedBlocks;
+    ParseResult _parsedBlocks;
 
     // The default type to assign to untyped blocks
     Type _defaultDeclType;

radiantcore/decl/DeclarationManager.cpp

@@ -206,6 +206,14 @@ void DeclarationManager::reloadDeclarations()
 
     runParsersForAllFolders();
 
+    // Process the buffered results synchronously
+    for (auto& [type, result] : _parseResults)
+    {
+        processParseResult(type, result);
+    }
+
+    _parseResults.clear();
+
     // Empty all declarations that haven't been touched during this reparse run
     {
         std::lock_guard declLock(_declarationLock);
@@ -495,22 +503,58 @@ void DeclarationManager::emitDeclsReloadedSignal(Type type, bool async)
     }
 }
 
-void DeclarationManager::onParserFinished(Type parserType, 
-    std::map<Type, std::vector<DeclarationBlockSyntax>>& parsedBlocks)
+void DeclarationManager::onParserFinished(Type parserType, ParseResult&& parsedBlocks)
+{
+    if (_reparseInProgress)
+    {
+        // In the reparse scenario the results are processed synchronously
+        // so buffer everything and let the reloadDeclarations() method
+        // assign the blocks in the thread that started it.
+        _parseResults.emplace_back(parserType, parsedBlocks);
+    }
+    else
+    {
+        // When not reloading, process the result immediately
+        processParseResult(parserType, parsedBlocks);
+    }
+
+    // Move the parser reference out and wait for it to finish. Once the parser is gone,
+    // the public API will be available to any callbacks the decls reloaded signal
+    // is going to invoke. Since we're already on the parser thread
+    // itself here, we need to do this on a separate thread to avoid deadlocks
+    {
+        std::lock_guard declLock(_declarationLock);
+
+        auto decls = _declarationsByType.find(parserType);
+        assert(decls != _declarationsByType.end());
+
+        // Move the parser reference from the dictionary as capture to the lambda
+        // Then let the unique_ptr in the lambda go out of scope to finish the thread
+        // Lambda is mutable to make the unique_ptr member non-const
+        decls->second.parserFinisher = std::async(std::launch::async, [p = std::move(decls->second.parser)]() mutable
+        {
+            p.reset();
+        });
+    }
+
+    if (!_reparseInProgress)
+    {
+        // Emit the signal on the same thread
+        emitDeclsReloadedSignal(parserType, false);
+    }
+}
+
+void DeclarationManager::processParseResult(Type parserType, ParseResult& parsedBlocks)
 {
     // Sort all parsed blocks into our main dictionary
     // unrecognised blocks will be pushed to _unrecognisedBlocks
     processParsedBlocks(parsedBlocks);
 
     // Process the list of unrecognised blocks (from this and any previous run)
     handleUnrecognisedBlocks();
-
-    // Invoke the declsReloaded signal for this type on a new thread
-    // to allow clients to use the API without deadlocking
-    emitDeclsReloadedSignal(parserType, true);
 }
 
-void DeclarationManager::processParsedBlocks(std::map<Type, std::vector<DeclarationBlockSyntax>>& parsedBlocks)
+void DeclarationManager::processParsedBlocks(ParseResult& parsedBlocks)
 {
     std::lock_guard declLock(_declarationLock);
     std::lock_guard creatorLock(_creatorLock);

radiantcore/decl/DeclarationManager.h

@@ -9,6 +9,7 @@
 #include "string/string.h"
 
 #include "DeclarationFile.h"
+#include "DeclarationFolderParser.h"
 
 namespace decl
 {
@@ -43,6 +44,8 @@ class DeclarationManager :
 
         // If not empty, holds the running parser
         std::unique_ptr<DeclarationFolderParser> parser;
+
+        std::future<void> parserFinisher;
     };
 
     // One entry for each decl
@@ -58,6 +61,9 @@ class DeclarationManager :
     std::size_t _parseStamp = 0;
     bool _reparseInProgress = false;
 
+    // Holds the results during reparseDeclarations
+    std::vector<std::pair<Type, ParseResult>> _parseResults;
+
 public:
     void registerDeclType(const std::string& typeName, const IDeclarationCreator::Ptr& parser) override;
     void unregisterDeclType(const std::string& typeName) override;
@@ -77,19 +83,19 @@ class DeclarationManager :
     void initialiseModule(const IApplicationContext& ctx) override;
     void shutdownModule() override;
 
-    // Invoked once a parser thread has finished. It will move its data over to here.
-    void onParserFinished(Type parserType,
-        std::map<Type, std::vector<DeclarationBlockSyntax>>& parsedBlocks);
+    // Invoked once a parser thread has finished (results are moved to here)
+    void onParserFinished(Type parserType, ParseResult&& parsedBlocks);
 
 private:
+    void processParseResult(Type parserType, ParseResult& parsedBlocks);
     void runParsersForAllFolders();
     void waitForTypedParsersToFinish();
 
     // Attempts to resolve the block type of the given block, returns true on success, false otherwise.
     // Stores the determined type in the given reference.
     std::map<std::string, Type, string::ILess> getTypenameMapping();
     bool tryDetermineBlockType(const DeclarationBlockSyntax& block, Type& type);
-    void processParsedBlocks(std::map<Type, std::vector<DeclarationBlockSyntax>>& parsedBlocks);
+    void processParsedBlocks(ParseResult& parsedBlocks);
 
     // Requires the creatorsMutex and the declarationMutex to be locked
     const IDeclaration::Ptr& createOrUpdateDeclaration(Type type, const DeclarationBlockSyntax& block);

radiantcore/particles/ParticleDef.cpp

@@ -157,7 +157,7 @@ void ParticleDef::parseFromTokens(parser::DefTokeniser& tokeniser)
     }
 }
 
-void ParticleDef::onParsingFinished()
+void ParticleDef::onSyntaxBlockAssigned(const decl::DeclarationBlockSyntax& block)
 {
     _changedSignal.emit();
 }

radiantcore/particles/ParticleDef.h

@@ -78,7 +78,7 @@ class ParticleDef :
 protected:
     void onBeginParsing() override;
 	void parseFromTokens(parser::DefTokeniser& tok) override;
-    void onParsingFinished() override;
+    void onSyntaxBlockAssigned(const decl::DeclarationBlockSyntax& block) override;
 
     std::string generateSyntax() override;