more threading issues

panda/src/display/graphicsThreadingModel.cxx

@@ -32,7 +32,7 @@
 //               Thus, for example, "cull/draw" indicates that the
 //               window will be culled in a thread called "cull", and
 //               drawn in a separate thread called "draw".
-//               "draw/draw" or simply "draw/" indicates the window
+//               "draw/draw" or simply "draw" indicates the window
 //               will be culled and drawn in the same thread, "draw".
 //               On the other hand, "/draw" indicates the thread will
 //               be culled in the main, or app thread, and drawn in a

panda/src/display/graphicsWindow.cxx

@@ -22,6 +22,7 @@
 #include "mouseButton.h"
 #include "keyboardButton.h"
 #include "mutexHolder.h"
+#include "reMutexHolder.h"
 #include "throw_event.h"
 
 TypeHandle GraphicsWindow::_type_handle;
@@ -77,7 +78,7 @@ WindowProperties GraphicsWindow::
 get_properties() const {
   WindowProperties result;
   {
-    MutexHolder holder(_properties_lock);
+    ReMutexHolder holder(_properties_lock);
     result = _properties;
   }
   return result;
@@ -95,7 +96,7 @@ WindowProperties GraphicsWindow::
 get_requested_properties() const {
   WindowProperties result;
   {
-    MutexHolder holder(_properties_lock);
+    ReMutexHolder holder(_properties_lock);
     result = _requested_properties;
   }
   return result;
@@ -109,7 +110,7 @@ get_requested_properties() const {
 ////////////////////////////////////////////////////////////////////
 void GraphicsWindow::
 clear_rejected_properties() {
-  MutexHolder holder(_properties_lock);
+  ReMutexHolder holder(_properties_lock);
   _rejected_properties.clear();
 }
 
@@ -126,7 +127,7 @@ WindowProperties GraphicsWindow::
 get_rejected_properties() const {
   WindowProperties result;
   {
-    MutexHolder holder(_properties_lock);
+    ReMutexHolder holder(_properties_lock);
     result = _rejected_properties;
   }
   return result;
@@ -145,7 +146,7 @@ get_rejected_properties() const {
 ////////////////////////////////////////////////////////////////////
 void GraphicsWindow::
 request_properties(const WindowProperties &requested_properties) {
-  MutexHolder holder(_properties_lock);
+  ReMutexHolder holder(_properties_lock);
   _requested_properties.add_properties(requested_properties);
 
   if (!_has_size && _requested_properties.has_size()) {
@@ -186,7 +187,7 @@ is_active() const {
 ////////////////////////////////////////////////////////////////////
 void GraphicsWindow::
 set_window_event(const string &window_event) {
-  MutexHolder holder(_properties_lock);
+  ReMutexHolder holder(_properties_lock);
   _window_event = window_event;
 }
 
@@ -200,7 +201,7 @@ set_window_event(const string &window_event) {
 string GraphicsWindow::
 get_window_event() const {
   string result;
-  MutexHolder holder(_properties_lock);
+  ReMutexHolder holder(_properties_lock);
   result = _window_event;
   return result;
 }
@@ -231,7 +232,7 @@ get_window_event() const {
 ////////////////////////////////////////////////////////////////////
 void GraphicsWindow::
 set_close_request_event(const string &close_request_event) {
-  MutexHolder holder(_properties_lock);
+  ReMutexHolder holder(_properties_lock);
   _close_request_event = close_request_event;
 }
 
@@ -247,7 +248,7 @@ set_close_request_event(const string &close_request_event) {
 string GraphicsWindow::
 get_close_request_event() const {
   string result;
-  MutexHolder holder(_properties_lock);
+  ReMutexHolder holder(_properties_lock);
   result = _close_request_event;
   return result;
 }
@@ -495,7 +496,7 @@ process_events() {
     // bitmask after all.
     WindowProperties properties;
     {
-      MutexHolder holder(_properties_lock);
+      ReMutexHolder holder(_properties_lock);
       properties = _requested_properties;
       _requested_properties.clear();
 
@@ -534,6 +535,7 @@ set_properties_now(WindowProperties &properties) {
       properties.get_open() != _properties.get_open()) {
     // Open or close a new window.  In this case we can get all of the
     // properties at once.
+
     _properties.add_properties(properties);
     properties.clear();
 
@@ -687,7 +689,7 @@ system_changed_properties(const WindowProperties &properties) {
       << "system_changed_properties(" << properties << ")\n";
   }
 
-  MutexHolder holder(_properties_lock);
+  ReMutexHolder holder(_properties_lock);
 
   if (properties.has_size()) {
     system_changed_size(properties.get_x_size(), properties.get_y_size());

panda/src/display/graphicsWindow.h

@@ -29,6 +29,7 @@
 #include "buttonEvent.h"
 #include "notify.h"
 #include "pmutex.h"
+#include "remutex.h"
 #include "pvector.h"
 
 ////////////////////////////////////////////////////////////////////
@@ -110,7 +111,7 @@ protected:
   WindowProperties _properties;
 
 private:
-  Mutex _properties_lock; 
+  ReMutex _properties_lock; 
   // protects _requested_properties, _rejected_properties, and
   // _window_event.
 

panda/src/express/pmutex.cxx

@@ -32,7 +32,7 @@ bool Mutex::
 debug_is_locked() const {
   return (_locking_thread == Thread::get_current_thread());
 }
-#endif
+#endif  // NDEBUG
 
 ////////////////////////////////////////////////////////////////////
 //     Function: Mutex::do_lock

panda/src/express/reMutex.I

@@ -121,3 +121,17 @@ get_lock_count() const {
   }
   return 0;
 }
+
+#ifndef NDEBUG
+////////////////////////////////////////////////////////////////////
+//     Function: ReMutex::debug_is_locked
+//       Access: Public
+//  Description: The same as is_locked().  This method name is
+//               provided just to maintain symmetry with Mutex (which
+//               doesn't provide an is_locked() method).
+////////////////////////////////////////////////////////////////////
+INLINE bool ReMutex::
+debug_is_locked() const {
+  return is_locked();
+}
+#endif  // NDEBUG

panda/src/express/reMutex.h

@@ -48,6 +48,10 @@ public:
   INLINE bool is_locked() const;
   INLINE int get_lock_count() const;
 
+#ifndef NDEBUG
+  INLINE bool debug_is_locked() const;
+#endif
+
 private:
   void do_lock();
   void do_release();

panda/src/pgraph/config_pgraph.cxx

@@ -397,6 +397,13 @@ init_libpgraph() {
   TransformState::register_with_read_factory();
   TransparencyAttrib::register_with_read_factory();
 
+  // By initializing the _states map up front, we also guarantee that
+  // the _states_lock mutex gets created before we spawn any threads
+  // (assuming no one is creating threads at static init time).
+  TransformState::init_states();
+  RenderState::init_states();
+  RenderEffects::init_states();
+
   LoaderFileTypeRegistry *reg = LoaderFileTypeRegistry::get_global_ptr();
   reg->register_type(new LoaderFileTypeBam);
 }

panda/src/pgraph/renderEffects.cxx

@@ -28,7 +28,10 @@
 #include "datagramIterator.h"
 #include "indent.h"
 #include "compareTo.h"
-
+#include "reMutexHolder.h"
+#include "thread.h"
+  
+ReMutex *RenderEffects::_states_lock = NULL;
 RenderEffects::States *RenderEffects::_states = NULL;
 CPT(RenderEffects) RenderEffects::_empty_state;
 TypeHandle RenderEffects::_type_handle;
@@ -43,12 +46,7 @@ TypeHandle RenderEffects::_type_handle;
 RenderEffects::
 RenderEffects() {
   if (_states == (States *)NULL) {
-    // Make sure the global _states map is allocated.  This only has
-    // to be done once.  We could make this map static, but then we
-    // run into problems if anyone creates a RenderState object at
-    // static init time; it also seems to cause problems when the
-    // Panda shared library is unloaded at application exit time.
-    _states = new States;
+    init_states();
   }
   _saved_entry = _states->end();
   _flags = 0;
@@ -83,6 +81,7 @@ operator = (const RenderEffects &) {
 RenderEffects::
 ~RenderEffects() {
   // Remove the deleted RenderEffects object from the global pool.
+  ReMutexHolder holder(*_states_lock);
   if (_saved_entry != _states->end()) {
     _states->erase(_saved_entry);
     _saved_entry = _states->end();
@@ -423,44 +422,6 @@ write(ostream &out, int indent_level) const {
   }
 }
 
-////////////////////////////////////////////////////////////////////
-//     Function: RenderEffects::cull_callback
-//       Access: Public
-//  Description: Calls cull_callback() on all effects.  You may check
-//               has_cull_callback() first to see if any effects
-//               define this method to do anything useful.
-////////////////////////////////////////////////////////////////////
-void RenderEffects::
-cull_callback(CullTraverser *trav, CullTraverserData &data,
-              CPT(TransformState) &node_transform,
-              CPT(RenderState) &node_state) const {
-  Effects::const_iterator ei;
-  for (ei = _effects.begin(); ei != _effects.end(); ++ei) {
-    (*ei)._effect->cull_callback(trav, data, node_transform, node_state);
-  }
-}
-
-////////////////////////////////////////////////////////////////////
-//     Function: RenderEffects::adjust_transform
-//       Access: Public
-//  Description: Calls adjust_transform() on all effects.  You may check
-//               has_adjust_transform() first to see if any effects
-//               define this method to do anything useful.
-//
-//               The order in which the individual effects are applied
-//               is not defined, so if more than one effect applies a
-//               change to the transform on any particular node, you
-//               might get indeterminate results.
-////////////////////////////////////////////////////////////////////
-void RenderEffects::
-adjust_transform(CPT(TransformState) &net_transform,
-                 CPT(TransformState) &node_transform) const {
-  Effects::const_iterator ei;
-  for (ei = _effects.begin(); ei != _effects.end(); ++ei) {
-    (*ei)._effect->adjust_transform(net_transform, node_transform);
-  }
-}
-
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffects::get_num_states
 //       Access: Published, Static
@@ -473,6 +434,7 @@ get_num_states() {
   if (_states == (States *)NULL) {
     return 0;
   }
+  ReMutexHolder holder(*_states_lock);
   return _states->size();
 }
 
@@ -506,6 +468,7 @@ validate_states() {
   if (_states->empty()) {
     return true;
   }
+  ReMutexHolder holder(*_states_lock);
 
   States::const_iterator si = _states->begin();
   States::const_iterator snext = si;
@@ -525,6 +488,68 @@ validate_states() {
   return true;
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffects::cull_callback
+//       Access: Public
+//  Description: Calls cull_callback() on all effects.  You may check
+//               has_cull_callback() first to see if any effects
+//               define this method to do anything useful.
+////////////////////////////////////////////////////////////////////
+void RenderEffects::
+cull_callback(CullTraverser *trav, CullTraverserData &data,
+              CPT(TransformState) &node_transform,
+              CPT(RenderState) &node_state) const {
+  Effects::const_iterator ei;
+  for (ei = _effects.begin(); ei != _effects.end(); ++ei) {
+    (*ei)._effect->cull_callback(trav, data, node_transform, node_state);
+  }
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffects::adjust_transform
+//       Access: Public
+//  Description: Calls adjust_transform() on all effects.  You may check
+//               has_adjust_transform() first to see if any effects
+//               define this method to do anything useful.
+//
+//               The order in which the individual effects are applied
+//               is not defined, so if more than one effect applies a
+//               change to the transform on any particular node, you
+//               might get indeterminate results.
+////////////////////////////////////////////////////////////////////
+void RenderEffects::
+adjust_transform(CPT(TransformState) &net_transform,
+                 CPT(TransformState) &node_transform) const {
+  Effects::const_iterator ei;
+  for (ei = _effects.begin(); ei != _effects.end(); ++ei) {
+    (*ei)._effect->adjust_transform(net_transform, node_transform);
+  }
+}
+  
+////////////////////////////////////////////////////////////////////
+//     Function: RenderEffects::init_states
+//       Access: Public, Static
+//  Description: Make sure the global _states map is allocated.  This
+//               only has to be done once.  We could make this map
+//               static, but then we run into problems if anyone
+//               creates a RenderEffects object at static init time;
+//               it also seems to cause problems when the Panda shared
+//               library is unloaded at application exit time.
+////////////////////////////////////////////////////////////////////
+void RenderEffects::
+init_states() {
+  _states = new States;
+
+  // TODO: we should have a global Panda mutex to allow us to safely
+  // create _states_lock without a startup race condition.  For the
+  // meantime, this is OK because we guarantee that this method is
+  // called at static init time, presumably when there is still only
+  // one thread in the world.
+  _states_lock = new ReMutex;
+  nassertv(Thread::get_current_thread() == Thread::get_main_thread());
+}
+  
+
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderEffects::return_new
 //       Access: Private, Static
@@ -541,16 +566,18 @@ CPT(RenderEffects) RenderEffects::
 return_new(RenderEffects *state) {
   nassertr(state != (RenderEffects *)NULL, state);
 
-  // This should be a newly allocated pointer, not one that was used
-  // for anything else.
-  nassertr(state->_saved_entry == _states->end(), state);
-
 #ifndef NDEBUG
   if (paranoid_const) {
     nassertr(validate_states(), state);
   }
 #endif
 
+  ReMutexHolder holder(*_states_lock);
+
+  // This should be a newly allocated pointer, not one that was used
+  // for anything else.
+  nassertr(state->_saved_entry == _states->end(), state);
+
   // Save the state in a local PointerTo so that it will be freed at
   // the end of this function if no one else uses it.
   CPT(RenderEffects) pt_state = state;
@@ -689,6 +716,7 @@ complete_pointers(TypedWritable **p_list, BamReader *manager) {
     nassertr(effect._effect != (RenderEffect *)NULL, pi);
     effect._type = effect._effect->get_type();
   }
+  ReMutexHolder holder(*_states_lock);
 
   // Now make sure the array is properly sorted.  (It won't
   // necessarily preserve its correct sort after being read from bam,

panda/src/pgraph/renderEffects.h

@@ -28,6 +28,7 @@
 #include "typedWritableReferenceCount.h"
 #include "pointerTo.h"
 #include "ordered_vector.h"
+#include "reMutex.h"
 
 class CullTraverser;
 class CullTraverserData;
@@ -108,6 +109,8 @@ public:
   virtual void adjust_transform(CPT(TransformState) &net_transform,
                                 CPT(TransformState) &node_transform) const;
 
+  static void init_states();
+
 private:
   static CPT(RenderEffects) return_new(RenderEffects *state);
   void determine_decal();
@@ -116,6 +119,10 @@ private:
   void determine_adjust_transform();
 
 private:
+  // This mutex protects _states.  It also protects any modification
+  // to the cache, which is encoded in _composition_cache and
+  // _invert_composition_cache.
+  static ReMutex *_states_lock;
   typedef pset<const RenderEffects *, indirect_less<const RenderEffects *> > States;
   static States *_states;
   static CPT(RenderEffects) _empty_state;

panda/src/pgraph/renderState.cxx

@@ -35,7 +35,10 @@
 #include "datagramIterator.h"
 #include "indent.h"
 #include "compareTo.h"
-
+#include "reMutexHolder.h"
+#include "thread.h"
+  
+ReMutex *RenderState::_states_lock = NULL;
 RenderState::States *RenderState::_states = NULL;
 CPT(RenderState) RenderState::_empty_state;
 UpdateSeq RenderState::_last_cycle_detect;
@@ -59,12 +62,7 @@ TypeHandle RenderState::_type_handle;
 RenderState::
 RenderState() {
   if (_states == (States *)NULL) {
-    // Make sure the global _states map is allocated.  This only has
-    // to be done once.  We could make this map static, but then we
-    // run into problems if anyone creates a RenderState object at
-    // static init time; it also seems to cause problems when the
-    // Panda shared library is unloaded at application exit time.
-    _states = new States;
+    init_states();
   }
   _saved_entry = _states->end();
   _flags = 0;
@@ -103,12 +101,13 @@ RenderState::
   nassertv(!is_destructing());
   set_destructing();
 
+  ReMutexHolder holder(*_states_lock);
   if (_saved_entry != _states->end()) {
     nassertv(_states->find(this) == _saved_entry);
     _states->erase(_saved_entry);
     _saved_entry = _states->end();
   }
-
+   
   remove_cache_pointers();
 
   // If this was true at the beginning of the destructor, but is no
@@ -319,6 +318,8 @@ compose(const RenderState *other) const {
     return this;
   }
 
+  ReMutexHolder holder(*_states_lock);
+
   // Is this composition already cached?
   CompositionCache::const_iterator ci = _composition_cache.find(other);
   if (ci != _composition_cache.end()) {
@@ -397,6 +398,8 @@ invert_compose(const RenderState *other) const {
     return make_empty();
   }
 
+  ReMutexHolder holder(*_states_lock);
+
   // Is this composition already cached?
   CompositionCache::const_iterator ci = _invert_composition_cache.find(other);
   if (ci != _invert_composition_cache.end()) {
@@ -590,6 +593,8 @@ get_override(TypeHandle type) const {
 ////////////////////////////////////////////////////////////////////
 int RenderState::
 unref() const {
+  ReMutexHolder holder(*_states_lock);
+
   if (get_cache_ref_count() > 0 &&
       get_ref_count() == get_cache_ref_count() + 1) {
     // If we are about to remove the one reference that is not in the
@@ -598,11 +603,6 @@ unref() const {
     // it exists.
 
     if (auto_break_cycles) {
-      // There might be a tiny race condition if multiple different
-      // threads perform cycle detects on related nodes at the same
-      // time.  But the cost of failing the race condition is low--we
-      // end up with a tiny leak that may eventually be discovered, big
-      // deal.
       ++_last_cycle_detect;
       if (r_detect_cycles(this, this, 1, _last_cycle_detect, NULL)) {
         // Ok, we have a cycle.  This will be a leak unless we break the
@@ -611,6 +611,7 @@ unref() const {
           pgraph_cat.debug()
             << "Breaking cycle involving " << (*this) << "\n";
         }
+
         ((RenderState *)this)->remove_cache_pointers();
       }
     }
@@ -684,6 +685,7 @@ get_num_states() {
   if (_states == (States *)NULL) {
     return 0;
   }
+  ReMutexHolder holder(*_states_lock);
   return _states->size();
 }
 
@@ -710,6 +712,7 @@ get_num_unused_states() {
   if (_states == (States *)NULL) {
     return 0;
   }
+  ReMutexHolder holder(*_states_lock);
 
   // First, we need to count the number of times each RenderState
   // object is recorded in the cache.
@@ -802,6 +805,7 @@ clear_cache() {
   if (_states == (States *)NULL) {
     return 0;
   }
+  ReMutexHolder holder(*_states_lock);
 
   PStatTimer timer(_cache_update_pcollector);
   int orig_size = _states->size();
@@ -878,6 +882,11 @@ clear_cache() {
 ////////////////////////////////////////////////////////////////////
 void RenderState::
 list_cycles(ostream &out) {
+  if (_states == (States *)NULL) {
+    return;
+  }
+  ReMutexHolder holder(*_states_lock);
+
   typedef pset<const RenderState *> VisitedStates;
   VisitedStates visited;
   CompositionCycleDesc cycle_desc;
@@ -928,6 +937,12 @@ list_cycles(ostream &out) {
 ////////////////////////////////////////////////////////////////////
 void RenderState::
 list_states(ostream &out) {
+  if (_states == (States *)NULL) {
+    out << "0 states:\n";
+    return;
+  }
+  ReMutexHolder holder(*_states_lock);
+
   out << _states->size() << " states:\n";
   States::const_iterator si;
   for (si = _states->begin(); si != _states->end(); ++si) {
@@ -948,6 +963,11 @@ list_states(ostream &out) {
 ////////////////////////////////////////////////////////////////////
 bool RenderState::
 validate_states() {
+  if (_states == (States *)NULL) {
+    return true;
+  }
+
+  ReMutexHolder holder(*_states_lock);
   if (_states->empty()) {
     return true;
   }
@@ -1021,7 +1041,7 @@ bin_removed(int bin_index) {
   // Do something here.
   nassertv(false);
 }
-
+  
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderState::return_new
 //       Access: Private, Static
@@ -1038,16 +1058,18 @@ CPT(RenderState) RenderState::
 return_new(RenderState *state) {
   nassertr(state != (RenderState *)NULL, state);
 
-  // This should be a newly allocated pointer, not one that was used
-  // for anything else.
-  nassertr(state->_saved_entry == _states->end(), state);
-
 #ifndef NDEBUG
   if (paranoid_const) {
     nassertr(validate_states(), state);
   }
 #endif
 
+  ReMutexHolder holder(*_states_lock);
+
+  // This should be a newly allocated pointer, not one that was used
+  // for anything else.
+  nassertr(state->_saved_entry == _states->end(), state);
+
   // Save the state in a local PointerTo so that it will be freed at
   // the end of this function if no one else uses it.
   CPT(RenderState) pt_state = state;
@@ -1273,12 +1295,13 @@ r_detect_cycles(const RenderState *start_state,
 //               particular RenderState.  The pointers to this
 //               object may be scattered around in the various
 //               CompositionCaches from other RenderState objects.
+//
+//               You must already be holding _states_lock before you
+//               call this method.
 ////////////////////////////////////////////////////////////////////
 void RenderState::
 remove_cache_pointers() {
-  // Now make sure we clean up all other floating pointers to the
-  // RenderState.  These may be scattered around in the various
-  // CompositionCaches from other RenderState objects.
+  nassertv(_states_lock->debug_is_locked());
 
   // Fortunately, since we added CompositionCache records in pairs, we
   // know exactly the set of RenderState objects that have us in their
@@ -1633,6 +1656,29 @@ update_pstats(int old_referenced_bits, int new_referenced_bits) {
   }
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: RenderState::init_states
+//       Access: Public, Static
+//  Description: Make sure the global _states map is allocated.  This
+//               only has to be done once.  We could make this map
+//               static, but then we run into problems if anyone
+//               creates a RenderState object at static init time;
+//               it also seems to cause problems when the Panda shared
+//               library is unloaded at application exit time.
+////////////////////////////////////////////////////////////////////
+void RenderState::
+init_states() {
+  _states = new States;
+
+  // TODO: we should have a global Panda mutex to allow us to safely
+  // create _states_lock without a startup race condition.  For the
+  // meantime, this is OK because we guarantee that this method is
+  // called at static init time, presumably when there is still only
+  // one thread in the world.
+  _states_lock = new ReMutex;
+  nassertv(Thread::get_current_thread() == Thread::get_main_thread());
+}
+
 
 ////////////////////////////////////////////////////////////////////
 //     Function: RenderState::register_with_read_factory

panda/src/pgraph/renderState.h

@@ -32,6 +32,7 @@
 #include "geomMunger.h"
 #include "weakPointerTo.h"
 #include "shaderExpansion.h"
+#include "reMutex.h"
 
 class GraphicsStateGuardianBase;
 class FogAttrib;
@@ -195,7 +196,14 @@ private:
   INLINE void consider_update_pstats(int old_referenced_bits) const;
   static void update_pstats(int old_referenced_bits, int new_referenced_bits);
 
+public:
+  static void init_states();
+
 private:
+  // This mutex protects _states.  It also protects any modification
+  // to the cache, which is encoded in _composition_cache and
+  // _invert_composition_cache.
+  static ReMutex *_states_lock;
   typedef pset<const RenderState *, indirect_less<const RenderState *> > States;
   static States *_states;
   static CPT(RenderState) _empty_state;

panda/src/pgraph/transformState.cxx

@@ -25,7 +25,10 @@
 #include "compareTo.h"
 #include "pStatTimer.h"
 #include "config_pgraph.h"
+#include "reMutexHolder.h"
+#include "thread.h"
 
+ReMutex *TransformState::_states_lock = NULL;
 TransformState::States *TransformState::_states = NULL;
 CPT(TransformState) TransformState::_identity_state;
 UpdateSeq TransformState::_last_cycle_detect;
@@ -47,12 +50,7 @@ TypeHandle TransformState::_type_handle;
 TransformState::
 TransformState() {
   if (_states == (States *)NULL) {
-    // Make sure the global _states map is allocated.  This only has
-    // to be done once.  We could make this map static, but then we
-    // run into problems if anyone creates a TransformState object at
-    // static init time; it also seems to cause problems when the
-    // Panda shared library is unloaded at application exit time.
-    _states = new States;
+    init_states();
   }
   _saved_entry = _states->end();
   _flags = F_is_identity | F_singular_known | F_is_2d;
@@ -97,12 +95,13 @@ TransformState::
     _inv_mat = (LMatrix4f *)NULL;
   }
 
+  ReMutexHolder holder(*_states_lock);
   if (_saved_entry != _states->end()) {
     nassertv(_states->find(this) == _saved_entry);
     _states->erase(_saved_entry);
     _saved_entry = _states->end();
   }
-
+   
   remove_cache_pointers();
 
   // If this was true at the beginning of the destructor, but is no
@@ -631,6 +630,8 @@ compose(const TransformState *other) const {
     return other;
   }
 
+  ReMutexHolder holder(*_states_lock);
+
   // Is this composition already cached?
   CompositionCache::const_iterator ci = _composition_cache.find(other);
   if (ci != _composition_cache.end()) {
@@ -717,6 +718,8 @@ invert_compose(const TransformState *other) const {
     return make_identity();
   }
 
+  ReMutexHolder holder(*_states_lock);
+
   // Is this composition already cached?
   CompositionCache::const_iterator ci = _invert_composition_cache.find(other);
   if (ci != _invert_composition_cache.end()) {
@@ -784,6 +787,8 @@ invert_compose(const TransformState *other) const {
 ////////////////////////////////////////////////////////////////////
 int TransformState::
 unref() const {
+  ReMutexHolder holder(*_states_lock);
+
   if (get_cache_ref_count() > 0 &&
       get_ref_count() == get_cache_ref_count() + 1) {
     // If we are about to remove the one reference that is not in the
@@ -792,11 +797,6 @@ unref() const {
     // it exists.
 
     if (auto_break_cycles) {
-      // There might be a tiny race condition if multiple different
-      // threads perform cycle detects on related nodes at the same
-      // time.  But the cost of failing the race condition is low--we
-      // end up with a tiny leak that may eventually be discovered, big
-      // deal.
       ++_last_cycle_detect;
       if (r_detect_cycles(this, this, 1, _last_cycle_detect, NULL)) {
         // Ok, we have a cycle.  This will be a leak unless we break the
@@ -805,6 +805,7 @@ unref() const {
           pgraph_cat.debug()
             << "Breaking cycle involving " << (*this) << "\n";
         }
+
         ((TransformState *)this)->remove_cache_pointers();
       }
     }
@@ -928,6 +929,7 @@ get_num_states() {
   if (_states == (States *)NULL) {
     return 0;
   }
+  ReMutexHolder holder(*_states_lock);
   return _states->size();
 }
 
@@ -954,6 +956,7 @@ get_num_unused_states() {
   if (_states == (States *)NULL) {
     return 0;
   }
+  ReMutexHolder holder(*_states_lock);
 
   // First, we need to count the number of times each TransformState
   // object is recorded in the cache.  We could just trust
@@ -1047,6 +1050,7 @@ clear_cache() {
   if (_states == (States *)NULL) {
     return 0;
   }
+  ReMutexHolder holder(*_states_lock);
 
   PStatTimer timer(_cache_update_pcollector);
   int orig_size = _states->size();
@@ -1123,6 +1127,11 @@ clear_cache() {
 ////////////////////////////////////////////////////////////////////
 void TransformState::
 list_cycles(ostream &out) {
+  if (_states == (States *)NULL) {
+    return;
+  }
+  ReMutexHolder holder(*_states_lock);
+
   typedef pset<const TransformState *> VisitedStates;
   VisitedStates visited;
   CompositionCycleDesc cycle_desc;
@@ -1173,6 +1182,12 @@ list_cycles(ostream &out) {
 ////////////////////////////////////////////////////////////////////
 void TransformState::
 list_states(ostream &out) {
+  if (_states == (States *)NULL) {
+    out << "0 states:\n";
+    return;
+  }
+  ReMutexHolder holder(*_states_lock);
+
   out << _states->size() << " states:\n";
   States::const_iterator si;
   for (si = _states->begin(); si != _states->end(); ++si) {
@@ -1193,6 +1208,11 @@ list_states(ostream &out) {
 ////////////////////////////////////////////////////////////////////
 bool TransformState::
 validate_states() {
+  if (_states == (States *)NULL) {
+    return true;
+  }
+
+  ReMutexHolder holder(*_states_lock);
   if (_states->empty()) {
     return true;
   }
@@ -1217,6 +1237,29 @@ validate_states() {
   return true;
 }
 
+////////////////////////////////////////////////////////////////////
+//     Function: TransformState::init_states
+//       Access: Public, Static
+//  Description: Make sure the global _states map is allocated.  This
+//               only has to be done once.  We could make this map
+//               static, but then we run into problems if anyone
+//               creates a TransformState object at static init time;
+//               it also seems to cause problems when the Panda shared
+//               library is unloaded at application exit time.
+////////////////////////////////////////////////////////////////////
+void TransformState::
+init_states() {
+  _states = new States;
+
+  // TODO: we should have a global Panda mutex to allow us to safely
+  // create _states_lock without a startup race condition.  For the
+  // meantime, this is OK because we guarantee that this method is
+  // called at static init time, presumably when there is still only
+  // one thread in the world.
+  _states_lock = new ReMutex;
+  nassertv(Thread::get_current_thread() == Thread::get_main_thread());
+}
+  
 ////////////////////////////////////////////////////////////////////
 //     Function: TransformState::return_new
 //       Access: Private, Static
@@ -1233,16 +1276,18 @@ CPT(TransformState) TransformState::
 return_new(TransformState *state) {
   nassertr(state != (TransformState *)NULL, state);
 
-  // This should be a newly allocated pointer, not one that was used
-  // for anything else.
-  nassertr(state->_saved_entry == _states->end(), state);
-
 #ifndef NDEBUG
   if (paranoid_const) {
     nassertr(validate_states(), state);
   }
 #endif
 
+  ReMutexHolder holder(*_states_lock);
+
+  // This should be a newly allocated pointer, not one that was used
+  // for anything else.
+  nassertr(state->_saved_entry == _states->end(), state);
+
   // Save the state in a local PointerTo so that it will be freed at
   // the end of this function if no one else uses it.
   CPT(TransformState) pt_state = state;
@@ -1544,9 +1589,14 @@ r_detect_cycles(const TransformState *start_state,
 //               particular TransformState.  The pointers to this
 //               object may be scattered around in the various
 //               CompositionCaches from other TransformState objects.
+//
+//               You must already be holding _states_lock before you
+//               call this method.
 ////////////////////////////////////////////////////////////////////
 void TransformState::
 remove_cache_pointers() {
+  nassertv(_states_lock->debug_is_locked());
+
   // Fortunately, since we added CompositionCache records in pairs, we
   // know exactly the set of TransformState objects that have us in their
   // cache: it's the same set of TransformState objects that we have in

panda/src/pgraph/transformState.h

@@ -29,6 +29,7 @@
 #include "updateSeq.h"
 #include "pStatCollector.h"
 #include "geomEnums.h"
+#include "reMutex.h"
 
 class GraphicsStateGuardianBase;
 class FactoryParams;
@@ -182,6 +183,9 @@ PUBLISHED:
   static void list_states(ostream &out);
   static bool validate_states();
 
+public:
+  static void init_states();
+
 private:
   class CompositionCycleDescEntry {
   public:
@@ -206,6 +210,10 @@ private:
   void remove_cache_pointers();
 
 private:
+  // This mutex protects _states.  It also protects any modification
+  // to the cache, which is encoded in _composition_cache and
+  // _invert_composition_cache.
+  static ReMutex *_states_lock;
   typedef phash_set<const TransformState *, indirect_less_hash<const TransformState *> > States;
   static States *_states;
   static CPT(TransformState) _identity_state;

panda/src/pstatclient/pStatClient.I

@@ -79,6 +79,7 @@ get_max_rate() const {
 ////////////////////////////////////////////////////////////////////
 INLINE int PStatClient::
 get_num_collectors() const {
+  ReMutexHolder holder(_lock);
   return _collectors.size();
 }
 
@@ -89,6 +90,7 @@ get_num_collectors() const {
 ////////////////////////////////////////////////////////////////////
 INLINE PStatCollectorDef *PStatClient::
 get_collector_def(int index) const {
+  ReMutexHolder holder(_lock);
   nassertr(index >= 0 && index < (int)_collectors.size(), NULL);
 
   return _collectors[index].get_def(this, index);
@@ -102,6 +104,7 @@ get_collector_def(int index) const {
 ////////////////////////////////////////////////////////////////////
 INLINE int PStatClient::
 get_num_threads() const {
+  ReMutexHolder holder(_lock);
   return _threads.size();
 }
 
@@ -112,6 +115,7 @@ get_num_threads() const {
 ////////////////////////////////////////////////////////////////////
 INLINE string PStatClient::
 get_thread_name(int index) const {
+  ReMutexHolder holder(_lock);
   nassertr(index >= 0 && index < (int)_threads.size(), string());
   return _threads[index]._name;
 }
@@ -183,6 +187,7 @@ resume_after_pause() {
 ////////////////////////////////////////////////////////////////////
 INLINE bool PStatClient::
 client_connect(string hostname, int port) {
+  ReMutexHolder holder(_lock);
   client_disconnect();
   return get_impl()->client_connect(hostname, port);
 }
@@ -194,6 +199,7 @@ client_connect(string hostname, int port) {
 ////////////////////////////////////////////////////////////////////
 INLINE bool PStatClient::
 client_is_connected() const {
+  ReMutexHolder holder(_lock);
   return has_impl() && _impl->client_is_connected();
 }
 
@@ -232,6 +238,7 @@ has_impl() const {
 ////////////////////////////////////////////////////////////////////
 INLINE PStatClientImpl *PStatClient::
 get_impl() {
+  ReMutexHolder holder(_lock);
   if (_impl == (PStatClientImpl *)NULL) {
     _impl = new PStatClientImpl(this);
   }

panda/src/pstatclient/pStatClient.cxx

@@ -93,6 +93,7 @@ PStatClient::
 ////////////////////////////////////////////////////////////////////
 PStatCollector PStatClient::
 get_collector(int index) const {
+  ReMutexHolder holder(_lock);
   nassertr(index >= 0 && index < (int)_collectors.size(), PStatCollector());
   return PStatCollector((PStatClient *)this, index);
 }
@@ -104,6 +105,7 @@ get_collector(int index) const {
 ////////////////////////////////////////////////////////////////////
 string PStatClient::
 get_collector_name(int index) const {
+  ReMutexHolder holder(_lock);
   nassertr(index >= 0 && index < (int)_collectors.size(), string());
 
   return _collectors[index].get_name();
@@ -119,6 +121,7 @@ get_collector_name(int index) const {
 ////////////////////////////////////////////////////////////////////
 string PStatClient::
 get_collector_fullname(int index) const {
+  ReMutexHolder holder(_lock);
   nassertr(index >= 0 && index < (int)_collectors.size(), string());
 
   int parent_index = _collectors[index].get_parent_index();
@@ -137,6 +140,7 @@ get_collector_fullname(int index) const {
 ////////////////////////////////////////////////////////////////////
 PStatThread PStatClient::
 get_thread(int index) const {
+  ReMutexHolder holder(_lock);
   nassertr(index >= 0 && index < (int)_threads.size(), PStatThread());
   return PStatThread((PStatClient *)this, index);
 }
@@ -189,6 +193,7 @@ main_tick() {
 ////////////////////////////////////////////////////////////////////
 void PStatClient::
 client_main_tick() {
+  ReMutexHolder holder(_lock);
   if (has_impl()) {
     _impl->client_main_tick();
   }
@@ -202,6 +207,7 @@ client_main_tick() {
 ////////////////////////////////////////////////////////////////////
 void PStatClient::
 client_disconnect() {
+  ReMutexHolder holder(_lock);
   if (has_impl()) {
     _impl->client_disconnect();
   }
@@ -254,6 +260,8 @@ get_global_pstats() {
 ////////////////////////////////////////////////////////////////////
 PStatCollector PStatClient::
 make_collector_with_relname(int parent_index, string relname) {
+  ReMutexHolder holder(_lock);
+
   if (relname.empty()) {
     relname = "Unnamed";
   }
@@ -292,6 +300,8 @@ make_collector_with_relname(int parent_index, string relname) {
 ////////////////////////////////////////////////////////////////////
 PStatCollector PStatClient::
 make_collector_with_name(int parent_index, const string &name) {
+  ReMutexHolder holder(_lock);
+
   nassertr(parent_index >= 0 && parent_index < (int)_collectors.size(),
            PStatCollector());
 
@@ -343,6 +353,8 @@ make_collector_with_name(int parent_index, const string &name) {
 ////////////////////////////////////////////////////////////////////
 PStatThread PStatClient::
 make_thread(const string &name) {
+  ReMutexHolder holder(_lock);
+
   ThingsByName::const_iterator ni =
     _threads_by_name.find(name);
 
@@ -388,6 +400,8 @@ make_thread(const string &name) {
 ////////////////////////////////////////////////////////////////////
 bool PStatClient::
 is_active(int collector_index, int thread_index) const {
+  ReMutexHolder holder(_lock);
+
   nassertr(collector_index >= 0 && collector_index < (int)_collectors.size(), false);
   nassertr(thread_index >= 0 && thread_index < (int)_threads.size(), false);
 
@@ -407,6 +421,8 @@ is_active(int collector_index, int thread_index) const {
 ////////////////////////////////////////////////////////////////////
 bool PStatClient::
 is_started(int collector_index, int thread_index) const {
+  ReMutexHolder holder(_lock);
+
   nassertr(collector_index >= 0 && collector_index < (int)_collectors.size(), false);
   nassertr(thread_index >= 0 && thread_index < (int)_threads.size(), false);
 
@@ -424,6 +440,8 @@ is_started(int collector_index, int thread_index) const {
 ////////////////////////////////////////////////////////////////////
 void PStatClient::
 start(int collector_index, int thread_index) {
+  ReMutexHolder holder(_lock);
+
 #ifdef _DEBUG
   nassertv(collector_index >= 0 && collector_index < (int)_collectors.size());
   nassertv(thread_index >= 0 && thread_index < (int)_threads.size());
@@ -451,6 +469,8 @@ start(int collector_index, int thread_index) {
 ////////////////////////////////////////////////////////////////////
 void PStatClient::
 start(int collector_index, int thread_index, float as_of) {
+  ReMutexHolder holder(_lock);
+
 #ifdef _DEBUG
   nassertv(collector_index >= 0 && collector_index < (int)_collectors.size());
   nassertv(thread_index >= 0 && thread_index < (int)_threads.size());
@@ -477,6 +497,8 @@ start(int collector_index, int thread_index, float as_of) {
 ////////////////////////////////////////////////////////////////////
 void PStatClient::
 stop(int collector_index, int thread_index) {
+  ReMutexHolder holder(_lock);
+
 #ifdef _DEBUG
   nassertv(collector_index >= 0 && collector_index < (int)_collectors.size());
   nassertv(thread_index >= 0 && thread_index < (int)_threads.size());
@@ -513,6 +535,8 @@ stop(int collector_index, int thread_index) {
 ////////////////////////////////////////////////////////////////////
 void PStatClient::
 stop(int collector_index, int thread_index, float as_of) {
+  ReMutexHolder holder(_lock);
+
 #ifdef _DEBUG
   nassertv(collector_index >= 0 && collector_index < (int)_collectors.size());
   nassertv(thread_index >= 0 && thread_index < (int)_threads.size());
@@ -551,6 +575,8 @@ stop(int collector_index, int thread_index, float as_of) {
 ////////////////////////////////////////////////////////////////////
 void PStatClient::
 clear_level(int collector_index, int thread_index) {
+  ReMutexHolder holder(_lock);
+
   _collectors[collector_index]._per_thread[thread_index]._has_level = false;
   _collectors[collector_index]._per_thread[thread_index]._level = 0.0;
 }
@@ -566,6 +592,8 @@ clear_level(int collector_index, int thread_index) {
 ////////////////////////////////////////////////////////////////////
 void PStatClient::
 set_level(int collector_index, int thread_index, float level) {
+  ReMutexHolder holder(_lock);
+
   // We don't want to condition this on whether the client is already
   // connected or the collector is already active, since we might
   // connect the client later, and we will want to have an accurate
@@ -588,6 +616,8 @@ set_level(int collector_index, int thread_index, float level) {
 ////////////////////////////////////////////////////////////////////
 void PStatClient::
 add_level(int collector_index, int thread_index, float increment) {
+  ReMutexHolder holder(_lock);
+
   increment *= get_collector_def(collector_index)->_factor;
   _collectors[collector_index]._per_thread[thread_index]._has_level = true;
   _collectors[collector_index]._per_thread[thread_index]._level += increment;
@@ -603,6 +633,8 @@ add_level(int collector_index, int thread_index, float increment) {
 ////////////////////////////////////////////////////////////////////
 float PStatClient::
 get_level(int collector_index, int thread_index) const {
+  ReMutexHolder holder(_lock);
+
   return _collectors[collector_index]._per_thread[thread_index]._level /
     get_collector_def(collector_index)->_factor;
 }

panda/src/pstatclient/pStatClient.h

@@ -24,6 +24,8 @@
 #include "pStatFrameData.h"
 #include "pStatClientImpl.h"
 #include "pStatCollectorDef.h"
+#include "reMutex.h"
+#include "reMutexHolder.h"
 #include "luse.h"
 #include "pmap.h"
 
@@ -113,6 +115,10 @@ private:
   void add_level(int collector_index, int thread_index, float increment);
   float get_level(int collector_index, int thread_index) const;
 
+private:
+  // This mutex protects everything in this class.
+  ReMutex _lock;
+
   // Not a phash_map, so the threads remain sorted by name.
   typedef pmap<string, int> ThingsByName;
   ThingsByName _threads_by_name;

panda/src/windisplay/winGraphicsWindow.cxx

@@ -997,22 +997,24 @@ window_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) {
         set_cursor_out_of_window();
         break;
     
-      case WM_CREATE: {
-        track_mouse_leaving(hwnd);
-        ClearToBlack(hwnd,_properties);
-    
-        POINT cpos;
-        GetCursorPos(&cpos);
-        ScreenToClient(hwnd,&cpos);
-        RECT clientRect;
-        GetClientRect(hwnd, &clientRect);
-        if(PtInRect(&clientRect,cpos))
-           set_cursor_in_window();  // should window focus be true as well?
-        else set_cursor_out_of_window();
-    
+      case WM_CREATE: 
+        {
+          track_mouse_leaving(hwnd);
+          ClearToBlack(hwnd, _properties);
+          
+          POINT cpos;
+          GetCursorPos(&cpos);
+          ScreenToClient(hwnd, &cpos);
+          RECT clientRect;
+          GetClientRect(hwnd, &clientRect);
+          if (PtInRect(&clientRect,cpos)) {
+            set_cursor_in_window();  // should window focus be true as well?
+          } else {
+            set_cursor_out_of_window();
+          }
+        }
         break;
-      }
-    
+        
       case WM_CLOSE:
         // This is a message from the system indicating that the user
         // has requested to close the window (e.g. alt-f4).