putil: Turn UniqueIdAllocator's free into an atomic operation (#999)

panda/src/putil/uniqueIdAllocator.cxx

@@ -194,20 +194,34 @@ is_allocated(uint32_t id) {
 /**
  * Free an allocated index (index must be between _min and _max that were
  * passed to the constructor).
+ *
+ * Since 1.11.0, returns true if the index has been freed successfully
+ * or false if the index has not been allocated yet, instead of
+ * triggering an assertion.
  */
-void UniqueIdAllocator::
+bool UniqueIdAllocator::
 free(uint32_t id) {
   LightMutexHolder holder(_lock);
 
   uniqueIdAllocator_debug("free("<<id<<")");
 
-  nassertv(id >= _min && id <= _max); // Attempt to free out-of-range id.
+  if (id < _min || id > _max) {
+    // Attempt to free out-of-range id.
+    return false;
+  }
+
   uint32_t index = id - _min; // Convert to _table index.
-  nassertv(_table[index] == IndexAllocated); // Attempt to free non-allocated id.
+
+  if (_table[index] != IndexAllocated) {
+    // Attempt to free non-allocated id.
+    return false;
+  }
+
   if (_next_free != IndexEnd) {
-    nassertv(_table[_last_free] == IndexEnd);
+    nassertr(_table[_last_free] == IndexEnd, false);
     _table[_last_free] = index;
   }
+
   _table[index] = IndexEnd; // Mark this element as the end of the list.
   _last_free = index;
 
@@ -217,6 +231,7 @@ free(uint32_t id) {
   }
 
   ++_free;
+  return true;
 }
 
 

panda/src/putil/uniqueIdAllocator.h

@@ -46,7 +46,7 @@ PUBLISHED:
 
   bool is_allocated(uint32_t index);
 
-  void free(uint32_t index);
+  bool free(uint32_t index);
   PN_stdfloat fraction_used() const;
 
   void output(std::ostream &out) const;

tests/putil/test_uniqueidallocator.py

@@ -75,7 +75,7 @@ def test_free():
 
     assert allocator.allocate() == 0
     assert allocator.is_allocated(0)
-    allocator.free(0)
+    assert allocator.free(0)
     assert not allocator.is_allocated(0)
 
 
@@ -87,7 +87,7 @@ def test_free_reallocation():
         assert allocator.is_allocated(i)
 
     for i in range(1, 5 + 1):
-        allocator.free(i)
+        assert allocator.free(i)
 
     for i in range(1, 5 + 1):
         assert not allocator.is_allocated(i)
@@ -96,6 +96,23 @@ def test_free_reallocation():
     assert allocator.allocate() == IndexEnd
 
 
+def test_free_unallocated():
+    allocator = UniqueIdAllocator(0, 2)
+
+    assert allocator.allocate() == 0
+    assert allocator.free(0)
+
+    for i in range(0, 2 + 1):
+        assert not allocator.free(i)
+
+
+def test_free_bounds():
+    allocator = UniqueIdAllocator(1, 3)
+
+    assert not allocator.free(0) # Out of range, left side
+    assert not allocator.free(4) # Out of range, right side
+
+
 def test_free_reallocation_mid():
     allocator = UniqueIdAllocator(1, 5)
 
@@ -103,8 +120,8 @@ def test_free_reallocation_mid():
         assert allocator.allocate() == i
         assert allocator.is_allocated(i)
 
-    allocator.free(2)
+    assert allocator.free(2)
-    allocator.free(3)
+    assert allocator.free(3)
 
     assert allocator.allocate() == 2
     assert allocator.allocate() == 3
@@ -115,7 +132,7 @@ def test_free_initial_reserve_id():
     allocator = UniqueIdAllocator(1, 3)
 
     allocator.initial_reserve_id(1)
-    allocator.free(1)
+    assert allocator.free(1)
     assert allocator.allocate() == 2
     assert allocator.allocate() == 3
     assert allocator.allocate() == 1