From 7f426ea64ed42fe0e9553c4bebd0b462b413b48a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Derzsi=20D=C3=A1niel?= <daniel@tohka.us>
Date: Sat, 29 Aug 2020 16:17:20 +0300
Subject: [PATCH] putil: Turn UniqueIdAllocator's free into an atomic operation
 (#999)

---
 panda/src/putil/uniqueIdAllocator.cxx | 23 +++++++++++++++++++----
 panda/src/putil/uniqueIdAllocator.h   |  2 +-
 tests/putil/test_uniqueidallocator.py | 27 ++++++++++++++++++++++-----
 3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/panda/src/putil/uniqueIdAllocator.cxx b/panda/src/putil/uniqueIdAllocator.cxx
index 942a17459f..d2204cfb09 100644
--- a/panda/src/putil/uniqueIdAllocator.cxx
+++ b/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;
 }
 
 
diff --git a/panda/src/putil/uniqueIdAllocator.h b/panda/src/putil/uniqueIdAllocator.h
index 66b1a1d876..313f569bb6 100644
--- a/panda/src/putil/uniqueIdAllocator.h
+++ b/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;
diff --git a/tests/putil/test_uniqueidallocator.py b/tests/putil/test_uniqueidallocator.py
index fca2b7f111..5ad897724e 100644
--- a/tests/putil/test_uniqueidallocator.py
+++ b/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)
-    allocator.free(3)
+    assert allocator.free(2)
+    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