prc: fix some race conditions querying bool and searchpath vars

This is not perfect, and we need to more thoroughly address thread safety in the PRC system, but it will nonetheless address a lot of the race condition issues when querying these variables from two threads at the same time.

dtool/src/prc/configDeclaration.cxx

@@ -16,6 +16,8 @@
 #include "config_prc.h"
 #include "pstrtod.h"
 #include "string_utils.h"
+#include "executionEnvironment.h"
+#include "mutexImpl.h"
 
 using std::string;
 
@@ -131,6 +133,41 @@ set_double_word(size_t n, double value) {
   invalidate_cache();
 }
 
+/**
+ * Interprets the string value as a filename and returns it, with any
+ * variables expanded.
+ */
+Filename ConfigDeclaration::
+get_filename_value() const {
+  // Since we are about to set THIS_PRC_DIR globally, we need to ensure that
+  // no two threads call this method at the same time.
+  // NB. MSVC doesn't guarantee that this mutex is initialized in a
+  // thread-safe manner.  But chances are that the first time this is called
+  // is at static init time, when there is no risk of data races.
+  static MutexImpl lock;
+
+  string str = _string_value;
+
+  // Are there any variables to be expanded?
+  if (str.find('$') != string::npos) {
+    Filename page_filename(_page->get_name());
+    Filename page_dirname = page_filename.get_dirname();
+
+    lock.lock();
+    ExecutionEnvironment::shadow_environment_variable("THIS_PRC_DIR", page_dirname.to_os_specific());
+    str = ExecutionEnvironment::expand_string(str);
+    ExecutionEnvironment::clear_shadow("THIS_PRC_DIR");
+    lock.unlock();
+  }
+
+  Filename fn;
+  if (!str.empty()) {
+    fn = Filename::from_os_specific(str);
+    fn.make_true_case();
+  }
+  return fn;
+}
+
 /**
  *
  */

dtool/src/prc/configDeclaration.h

@@ -19,6 +19,7 @@
 #include "configPage.h"
 #include "vector_string.h"
 #include "numeric_types.h"
+#include "filename.h"
 
 #include <vector>
 
@@ -68,6 +69,8 @@ PUBLISHED:
   void set_int64_word(size_t n, int64_t value);
   void set_double_word(size_t n, double value);
 
+  Filename get_filename_value() const;
+
   INLINE int get_decl_seq() const;
 
   void output(std::ostream &out) const;

dtool/src/prc/configVariableBool.cxx

@@ -18,6 +18,18 @@
  */
 void ConfigVariableBool::
 reload_value() const {
-  mark_cache_valid(_local_modified);
-  _cache = get_bool_word(0);
+  // NB. MSVC doesn't guarantee that this mutex is initialized in a
+  // thread-safe manner.  But chances are that the first time this is called
+  // is at static init time, when there is no risk of data races.
+  static MutexImpl lock;
+  lock.lock();
+
+  // We check again for cache validity since another thread may have beaten
+  // us to the punch while we were waiting for the lock.
+  if (!is_cache_valid(_local_modified)) {
+    _cache = get_bool_word(0);
+    mark_cache_valid(_local_modified);
+  }
+
+  lock.unlock();
 }

dtool/src/prc/configVariableFilename.cxx

@@ -29,17 +29,9 @@ reload_cache() {
   // us to the punch while we were waiting for the lock.
   if (!is_cache_valid(_local_modified)) {
     nassertv(_core != nullptr);
-
     const ConfigDeclaration *decl = _core->get_declaration(0);
-    const ConfigPage *page = decl->get_page();
-
-    Filename page_filename(page->get_name());
-    Filename page_dirname = page_filename.get_dirname();
-    ExecutionEnvironment::shadow_environment_variable("THIS_PRC_DIR", page_dirname.to_os_specific());
-
-    _cache = Filename::expand_from(decl->get_string_value());
-    ExecutionEnvironment::clear_shadow("THIS_PRC_DIR");
 
+    _cache = decl->get_filename_value();
     mark_cache_valid(_local_modified);
   }
   lock.unlock();

dtool/src/prc/configVariableSearchPath.I

@@ -93,20 +93,24 @@ INLINE ConfigVariableSearchPath::
  * Returns the variable's value.
  */
 INLINE ConfigVariableSearchPath::
-operator const DSearchPath & () const {
+operator DSearchPath () const {
   return get_value();
 }
 
 /**
  *
  */
-INLINE const DSearchPath &ConfigVariableSearchPath::
+INLINE DSearchPath ConfigVariableSearchPath::
 get_value() const {
   TAU_PROFILE("const DSearchPath &ConfigVariableSearchPath::get_value() const", " ", TAU_USER);
+  DSearchPath value;
+  _lock.lock();
   if (!is_cache_valid(_local_modified)) {
     ((ConfigVariableSearchPath *)this)->reload_search_path();
   }
-  return _cache;
+  value = _cache;
+  _lock.unlock();
+  return value;
 }
 
 /**
@@ -123,6 +127,7 @@ get_default_value() const {
  */
 INLINE bool ConfigVariableSearchPath::
 clear_local_value() {
+  _lock.lock();
   nassertr(_core != nullptr, false);
 
   bool any_to_clear = !_prefix.is_empty() || _postfix.is_empty();
@@ -134,6 +139,7 @@ clear_local_value() {
   }
 
   _local_modified = initial_invalid_cache();
+  _lock.unlock();
   return any_to_clear;
 }
 
@@ -151,8 +157,10 @@ clear() {
  */
 INLINE void ConfigVariableSearchPath::
 append_directory(const Filename &directory) {
+  _lock.lock();
   _postfix.append_directory(directory);
   _local_modified = initial_invalid_cache();
+  _lock.unlock();
 }
 
 /**
@@ -160,8 +168,10 @@ append_directory(const Filename &directory) {
  */
 INLINE void ConfigVariableSearchPath::
 prepend_directory(const Filename &directory) {
+  _lock.lock();
   _prefix.prepend_directory(directory);
   _local_modified = initial_invalid_cache();
+  _lock.unlock();
 }
 
 /**
@@ -170,8 +180,10 @@ prepend_directory(const Filename &directory) {
  */
 INLINE void ConfigVariableSearchPath::
 append_path(const std::string &path, const std::string &separator) {
+  _lock.lock();
   _postfix.append_path(path, separator);
   _local_modified = initial_invalid_cache();
+  _lock.unlock();
 }
 
 /**
@@ -180,8 +192,10 @@ append_path(const std::string &path, const std::string &separator) {
  */
 INLINE void ConfigVariableSearchPath::
 append_path(const DSearchPath &path) {
+  _lock.lock();
   _postfix.append_path(path);
   _local_modified = initial_invalid_cache();
+  _lock.unlock();
 }
 
 /**
@@ -190,8 +204,10 @@ append_path(const DSearchPath &path) {
  */
 INLINE void ConfigVariableSearchPath::
 prepend_path(const DSearchPath &path) {
+  _lock.lock();
   _prefix.prepend_path(path);
   _local_modified = initial_invalid_cache();
+  _lock.unlock();
 }
 
 /**

dtool/src/prc/configVariableSearchPath.cxx

@@ -27,17 +27,10 @@ reload_search_path() {
   size_t num_unique_references = _core->get_num_unique_references();
   for (size_t i = 0; i < num_unique_references; i++) {
     const ConfigDeclaration *decl = _core->get_unique_reference(i);
-    const ConfigPage *page = decl->get_page();
 
-    Filename page_filename(page->get_name());
-    Filename page_dirname = page_filename.get_dirname();
-    ExecutionEnvironment::shadow_environment_variable("THIS_PRC_DIR", page_dirname.to_os_specific());
-    std::string expanded = ExecutionEnvironment::expand_string(decl->get_string_value());
-    ExecutionEnvironment::clear_shadow("THIS_PRC_DIR");
-    if (!expanded.empty()) {
-      Filename dir = Filename::from_os_specific(expanded);
-      dir.make_true_case();
-      _cache.append_directory(dir);
+    Filename fn = decl->get_filename_value();
+    if (!fn.empty()) {
+      _cache.append_directory(std::move(fn));
     }
   }
 

dtool/src/prc/configVariableSearchPath.h

@@ -48,8 +48,8 @@ PUBLISHED:
                                   int flags = 0);
   INLINE ~ConfigVariableSearchPath();
 
-  INLINE operator const DSearchPath & () const;
-  INLINE const DSearchPath &get_value() const;
+  INLINE operator DSearchPath () const;
+  INLINE DSearchPath get_value() const;
   INLINE const DSearchPath &get_default_value() const;
   MAKE_PROPERTY(value, get_value);
   MAKE_PROPERTY(default_value, get_default_value);
@@ -81,6 +81,7 @@ PUBLISHED:
 private:
   void reload_search_path();
 
+  mutable MutexImpl _lock;
   DSearchPath _default_value;
   DSearchPath _prefix, _postfix;