#6093: Further tweak the DeclarationManager to wait for all parsers t…

…o finish before serving any queries. Since decls like Tables (or stricly speaking any decl) cannot be pinned down to a single directory, any running parser might be coming home with the decl type/name combination the client is looking for.
codereader · Sep 11, 2022 · 09614dd · 09614dd
1 parent 24399fa
commit 09614dd
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 35 deletions.
diff --git a/radiantcore/decl/DeclarationManager.cpp b/radiantcore/decl/DeclarationManager.cpp
@@ -156,29 +156,16 @@ void DeclarationManager::foreachDeclaration(Type type, const std::function<void(
 
 void DeclarationManager::doWithDeclarationLock(Type type, const std::function<void(NamedDeclarations&)>& action)
 {
+    // All parsers should be done before doing anything with the declarations
+    waitForTypedParsersToFinish();
+
     // Find type dictionary
-    auto declLock = std::make_unique<std::lock_guard<std::recursive_mutex>>(_declarationAndCreatorLock);
+    std::lock_guard declLock(_declarationAndCreatorLock);
 
     auto decls = _declarationsByType.find(type);
 
     if (decls == _declarationsByType.end()) return;
 
-    // Ensure the parser is done
-    if (decls->second.parser)
-    {
-        // Move the parser to prevent the parser thread from trying to do
-        // the same in onParserFinished
-        std::unique_ptr parser(std::move(decls->second.parser));
-
-        // Release the lock and let the thread finish
-        declLock.reset();
-
-        parser->ensureFinished(); // blocks
-        parser.reset();
-
-        declLock = std::make_unique<std::lock_guard<std::recursive_mutex>>(_declarationAndCreatorLock);
-    }
-
     action(decls->second.decls);
 }
 

diff --git a/test/DeclManager.cpp b/test/DeclManager.cpp
@@ -128,7 +128,7 @@ class TestDeclarationCreator :
     public decl::IDeclarationCreator
 {
 public:
-    bool processingDisabled = false;
+    std::function<void()> creationCallback;
 
     decl::Type getDeclType() const override
     {
@@ -137,10 +137,9 @@ class TestDeclarationCreator :
 
     decl::IDeclaration::Ptr createDeclaration(const std::string& name) override
     {
-        while (processingDisabled)
+        if (creationCallback)
         {
-            using namespace std::chrono_literals;
-            std::this_thread::sleep_for(20ms);
+            creationCallback();
         }
 
         return std::make_shared<TestDeclaration>(getDeclType(), name);
@@ -303,29 +302,33 @@ TEST_F(DeclManagerTest, CreatorRegistrationDuringRunningThread)
 {
     auto creator = std::make_shared<TestDeclarationCreator>();
 
-    // Hold back this creator until we let it go in this fixture
-    creator->processingDisabled = true;
+    bool testDecl2Registered = false;
 
-    GlobalDeclarationManager().registerDeclType("testdecl", creator);
+    // When the first testdecl is created, we register the decl2 type
+    creator->creationCallback = [&]()
+    {
+        if (!testDecl2Registered)
+        {
+            // Register the testdecl2 creator now, it should be used by the decl manager
+            // (after the running thread is done) to parse the missing pieces
+            GlobalDeclarationManager().registerDeclType("testdecl2", std::make_shared<TestDeclaration2Creator>());
+            testDecl2Registered = true;
+        }
+    };
 
-    // Parse this folder, it contains decls of type testdecl and testdecl2 in the .decl files
-    // The creator will block and keep the thread alive until further notice
-    GlobalDeclarationManager().registerDeclFolder(decl::Type::TestDecl, TEST_DECL_FOLDER, ".decl");
+    GlobalDeclarationManager().registerDeclType("testdecl", creator);
 
+    // We assume we know nothing about the decl2 typed table yet
     auto foundTestDecl2Names = getAllDeclNames(decl::Type::TestDecl2);
     EXPECT_FALSE(foundTestDecl2Names.count("decltable1") > 0);
 
-    // Register the testdecl2 creator now, it should be used by the decl manager to parse the missing pieces
-    GlobalDeclarationManager().registerDeclType("testdecl2", std::make_shared<TestDeclaration2Creator>());
-
-    // The first thread is still running, so we didn't get the decltable1 decls yet
-    foundTestDecl2Names = getAllDeclNames(decl::Type::TestDecl2);
-    EXPECT_FALSE(foundTestDecl2Names.count("decltable1") > 0);
+    // Parse this folder, it contains decls of type testdecl and testdecl2 in the .decl files
+    GlobalDeclarationManager().registerDeclFolder(decl::Type::TestDecl, TEST_DECL_FOLDER, ".decl");
 
-    // Released the flag to let the first parser finish its work
-    creator->processingDisabled = false;
     getAllDeclNames(decl::Type::TestDecl);
 
+    EXPECT_TRUE(testDecl2Registered) << "Callback to register testdecl2 has never been invoked";
+
     // Everything should be registered now
     checkKnownTestDecl2Names();
 }