TypeManager.pointer_attribute cannot be safely used in nested CustomType. #480

Activity

1. jordanbriere commented on Jul 3, 2023

   

   jordanbriere

   on Jul 3, 2023

   Contributor

   More actions

   test.test2.size = 1 # BROKEN!, the memory of size is already deallocated.

   That's not entirely accurate. The memory allocated for size is not freed yet because its setter still hold test2. After the assignment is done, however, test2 goes out of scope along with everything it allocated. For example:

   test = Test()
   
   test2 = test.test2
   test2.size = 1
   
   # Here test2.size is still held by test2._allocated_pointers...
   del test2
   # ...now size is being deallocated because test2 went out of scope.

   So, the problem is not really pointer_attribute itself, but the fact that instance_attribute don't hold converted values:

   Source.Python/addons/source-python/packages/source-python/memory/manager.py

   Lines 446 to 450 in f129713

   	def fget(ptr):
   	"""Return the instance attribute value."""
   	# Handle custom type
   	if not native_type:
   	return self.convert(type_name, ptr + offset)

   If test2 was a pointer_attribute it would be held by test._pointer_values on assignment. Same goes for test2.other_test that is being held by test2._pointer_values which are also freed when test2 is no more. For instance, making Test.test2 cached would fix these issues:

   class Test(CustomType, metaclass=manager):
       _size = 12
   
       test2 = cached_property(manager.instance_attribute("Test2", 4))

   Because now test2 lifetime would be dependant of test itself.
2. CookStar commented on Aug 10, 2023

   

   CookStar

   on Aug 10, 2023

   ContributorAuthor

   More actions

   That's not entirely accurate. The memory allocated for size is not freed yet because its setter still hold test2. After the assignment is done, however, test2 goes out of scope along with everything it allocated.

   I know, that is exactly what I was explaining in the comment I wrote.

   class Test(CustomType, metaclass=manager):
       _size = 12
   
       test2 = cached_property(manager.instance_attribute("Test2", 4))

   First, cached_property accepts getter and setter, not property, second, it is incompatible with data.ini, and third, that's just shuffling Python objects around to extend its lifetime. I understand objects lifetime, but others don't. That method cannot be used to prevent crashes.

   The fundamental problem is that the _allocated_pointers and _pointer_values of CustomType are managing Python objects that should not be managed. Even if we use cached_property or some other method, there is no guarantee that the original Test2 can manage other objects in the first place (i.e. Test2 cannot contain Python objects since they can be created by make_object() at any time).

   I wrote about the fundamental problem in #490, so I'll close this one.
3. CookStar

   closed this as not plannedon Aug 10, 2023
4. jordanbriere commented on Aug 10, 2023

   

   jordanbriere

   on Aug 10, 2023

   Last edited by jordanbriere

   Contributor

   More actions

   First, cached_property accepts getter and setter, not property,

   cached_property(property) is the equivalence of CachedProperty(property.__get__. property.__set__, property.__delete__, property.__doc__) (via CachedProperty.wrap_descriptor).
5. CookStar commented on Aug 10, 2023

   

   CookStar

   on Aug 10, 2023

   ContributorAuthor

   More actions

   cached_property(property) is the equivalence of CachedProperty(property.__get__. property.__set__, property.__delete__, property.__doc__) (via CachedProperty.wrap_descriptor).

   It seems I was testing with an old binary, my bad. But as noted above, cached_property is not a solution to this problem.