Conversation
|
Another option would be to use a |
I think refcell would be single threaded making multi-threaded application not working... |
|
Good point! @haixuanTao What needs to happen to get this merged? Are you happy with it or do you want to consider alternative implementations? |
apis/python/node/src/lib.rs
Outdated
| Ok(Node { | ||
| events: Events { | ||
| inner: EventsInner::Dora(events), | ||
| inner: Mutex::new(EventsInner::Dora(events)), |
There was a problem hiding this comment.
I think Arc Mutex is better in this case for multi-threaded use cases
| inner: Mutex::new(EventsInner::Dora(events)), | |
| inner: Arc::new(Mutex::new(EventsInner::Dora(events))), |
There was a problem hiding this comment.
What would be the advantage of adding an Arc here? Is there a way to clone the event channel that I don't see?
There was a problem hiding this comment.
Well, yes you could have python cloning the node within Python.
I don't know if python reference counting is enough for making this thread safe.
There was a problem hiding this comment.
AFAIK pyo3 already wraps the whole thing into some reference counted wrapper. So I would assume that the new Arc here will always have a reference count of exactly 1.
|
I have been thinking about this and I will merge it once we have Arc> to make sure we can safely share nodes between thread! Thanks for the PR! |
| pub fn get_mut(&mut self) -> std::sync::MutexGuard<T> { | ||
| pub fn get_mut(&self) -> std::sync::MutexGuard<T> { | ||
| self.0.try_lock().expect("failed to lock DelayedCleanup") | ||
| } |
There was a problem hiding this comment.
Note that this will still panic on concurrent send operations. This PR seems to be mostly about supporting send and receive concurrently, so I don't think that we need to fix this as part of the same PR. I just wanted to mention it.
|
@haixuanTao I wrapped the new Mutex in an ARC and fixed a merge conflict with main. |
|
Thanks a lot! |
Allows concurrent read/write access through a node (to address #748). "Already borrowed" seems to happen when there are concurrent calls to read/write functions because both currently require an exclusive
Noderef.DelayedCleanup.get_mutand callers to use&selfinstead of&mut self.Events.innerso callers can use&selfinstead of&mut self.