Threadsafety in string dtype
seberg opened this issue · 5 comments
I don't want anyone to waste cycles on this at this point. However, I do not think that the current string dtype implementation is fully thread-safe if one thread mutates the array while another thread also works with it (reading or writing).
Some form of locking seems necessary. This happens when mutating an element, the original one gets free'd so if someone still using it, that would be incorrect.
That could lead to a double-free, but overall the whole "store" operation probably needs to block (both write-lock blocks everything, read-lock blocks writes).
Not sure how this is usually implement. The probability of contesting access should be practically 0, so I would suspect a fine-grained lock that assumes contesting never really happens is best?
(That said, I am not sure mpfdtype is fully either, maybe it is possible for a number to be partially updated, which might mean it sees neither the old nor the new value :).)
If we end up deciding to make string array elements immutable then we also get thread safety for free, I think?
You mean the allocated string or the data stored on the array itself (making it effectively write-only always?). Otherwise, I think there is still some issue in theory when replacing the string.
I'm thinking in terms of the design described in #76, the array buffer would store a strided int32 index into the string data that would be stored contiguously in a single heap allocation attached to the descriptor object. In that design I think both buffers would need to be immutable, so the array would be read-only after it gets created and the string data and offsets would survive unmodified as long as the array does.
Looking at this again, I think the first thing to try is to add a PyThread_type_lock mutex to the arena or allocator struct. If that adds significant single-threaded overhead we're going to need to try something fancier as a first pass.
This is fixed now. We have a mutex locking access to the allocator along with tests for multithreaded access and mutation.