Merge pull request #3038 from zerebubuth/fix/deadlock-in-recursive-datasource-registration

Fix deadlock in recursive datasource registration.

include/mapnik/datasource_cache.hpp

@@ -33,6 +33,7 @@
 #include <set>
 #include <vector>
 #include <memory>
+#include <mutex>
 
 namespace mapnik {
 
@@ -56,6 +57,11 @@ private:
     ~datasource_cache();
     std::map<std::string,std::shared_ptr<PluginInfo> > plugins_;
     std::set<std::string> plugin_directories_;
+    // the singleton has a mutex protecting the instance pointer,
+    // but the instance also needs its own mutex to protect the
+    // plugins_ and plugin_directories_ members which are potentially
+    // modified recusrively by register_datasources(path, true);
+    std::recursive_mutex instance_mutex_;
 };
 
 extern template class MAPNIK_DECL singleton<datasource_cache, CreateStatic>;

src/datasource_cache.cpp

@@ -88,7 +88,7 @@ datasource_ptr datasource_cache::create(parameters const& params)
     // add scope to ensure lock is released asap
     {
 #ifdef MAPNIK_THREADSAFE
-        std::lock_guard<std::mutex> lock(mutex_);
+        std::lock_guard<std::recursive_mutex> lock(instance_mutex_);
 #endif
         itr=plugins_.find(*type);
         if (itr == plugins_.end())
@@ -132,6 +132,9 @@ datasource_ptr datasource_cache::create(parameters const& params)
 
 std::string datasource_cache::plugin_directories()
 {
+#ifdef MAPNIK_THREADSAFE
+    std::lock_guard<std::recursive_mutex> lock(instance_mutex_);
+#endif
     return boost::algorithm::join(plugin_directories_,", ");
 }
 
@@ -143,6 +146,10 @@ std::vector<std::string> datasource_cache::plugin_names()
     names = get_static_datasource_names();
 #endif
 
+#ifdef MAPNIK_THREADSAFE
+    std::lock_guard<std::recursive_mutex> lock(instance_mutex_);
+#endif
+
     std::map<std::string,std::shared_ptr<PluginInfo> >::const_iterator itr;
     for (itr = plugins_.begin(); itr != plugins_.end(); ++itr)
     {
@@ -155,7 +162,7 @@ std::vector<std::string> datasource_cache::plugin_names()
 bool datasource_cache::register_datasources(std::string const& dir, bool recurse)
 {
 #ifdef MAPNIK_THREADSAFE
-    std::lock_guard<std::mutex> lock(mutex_);
+    std::lock_guard<std::recursive_mutex> lock(instance_mutex_);
 #endif
     if (!mapnik::util::exists(dir))
     {
@@ -202,6 +209,9 @@ bool datasource_cache::register_datasources(std::string const& dir, bool recurse
 
 bool datasource_cache::register_datasource(std::string const& filename)
 {
+#ifdef MAPNIK_THREADSAFE
+    std::lock_guard<std::recursive_mutex> lock(instance_mutex_);
+#endif
     try
     {
         if (!mapnik::util::exists(filename))

test/standalone/datasource_registration_test.cpp

@@ -0,0 +1,46 @@
+#define CATCH_CONFIG_MAIN
+#include "catch.hpp"
+
+#include <mapnik/datasource_cache.hpp>
+#include <mapnik/debug.hpp>
+
+#include <iostream>
+#include <vector>
+#include <algorithm>
+
+TEST_CASE("datasource_cache") {
+
+SECTION("registration") {
+    try
+    {
+        mapnik::logger logger;
+        mapnik::logger::severity_type original_severity = logger.get_severity();
+        bool success = false;
+        auto &cache = mapnik::datasource_cache::instance();
+
+        // registering a directory without any plugins should return false
+        success = cache.register_datasources("test/data/vrt");
+        CHECK(success == false);
+
+        // registering a directory for the first time should return true
+        success = cache.register_datasources("plugins/input");
+        REQUIRE(success == true);
+
+        // registering the same directory again should now return false
+        success = cache.register_datasources("plugins/input");
+        CHECK(success == false);
+
+        // registering the same directory again, but recursively should
+        // still return false - even though there are subdirectories, they
+        // do not contain any more plugins.
+        success = cache.register_datasources("plugins/input", true);
+        CHECK(success == false);
+    }
+    catch (std::exception const & ex)
+    {
+        std::clog << ex.what() << "\n";
+        REQUIRE(false);
+    }
+
+}
+}