Fix deadlock in recursive datasource registration.

The datasource cache was taking an exclusive lock on the simple
mutex used to protect the singleton's data pointer. This works
okay when everyone always calls it non-recursively, but when the
recursive flag is true then it will always deadlock when called
on any directory with subdirectories.

Additionally, many methods which accessed private data members of
the cache were not protected by any locks.

Since the call pattern of registering datasources is strictly
tree-shaped then it's a good candidate for a recursive mutex. This
has a slightly higher overhead than a simple mutex, so rather than
change the singleton's mutex to be recursive, I've added a new
instance mutex to the datasource cache.

Also, added a very basic test which reproduces the problem and
shows that it's fixed with this patch.

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);
+    }
+
+}
+}