RNA: Protect accessors that write data with a global mutex.
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on Jul 28 2021, 5:50 PM.

Details

Reviewers

Brecht Van Lommel (brecht)

Maniphest Tasks

Commits

Summary

Ideally accessors should never create or modify data, but there are some
cases where this bad behavior is currently unavoidable.

This is the case of the Pointer accessor when the actual IDProperty has
not yet been created.

There is now a global mutex (LOCK_RNA) to make such cases thread-safe.

NOTE: this fixes a memory leak in liboverride diffing process when several different overrides use a same linked reference ID.

source/blender/blenlib/BLI_threads.h
77 ↗	(On Diff #40073)	Not sure about the name, maybe `LOCK_RNA_READ` would be better? To enforce this should only be used in some exceptional accessor case?

source/blender/blenlib/BLI_threads.h
77 ↗	(On Diff #40073)	This global list of locks should be avoided, just add a local `static ThreadMutex lock = BLI_MUTEX_INITIALIZER;` in the function.
source/blender/makesrna/intern/rna_access.c
3707–3708	Can we lock only when the pointer does not exist yet, inside `RNA_property_pointer_add`? We might access this many times, but only rarely do we actually need to allocate something.

This revision now requires changes to proceed.Jul 28 2021, 6:09 PM

Updated from review.

source/blender/makesrna/intern/rna_access.c
3707–3708	Not sure I understand that comment? `RNA_pointer_add()` also calls `RNA_property_pointer_add`, so adding the lock inside `RNA_property_pointer_add` would actually make it used more often? Further more, we already checked at the beginning of that function (`RNA_property_pointer_get`) that this is not currently allocated (the `idprop = rna_idproperty_check(&prop, ptr)` in the first `if`) ? So I think locking here is the best solution? Also missing a comment about why we do not check again for existence after locking here (since it's done by `RNA_property_pointer_add` itself), will add with update for the other remarks.