-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Protect document state with recursive lock and add objectDidChange publisher #192
Conversation
try work() | ||
} | ||
#endif | ||
|
||
#if canImport(Combine) | ||
public let objectDidChange: PassthroughSubject<(), Never> = .init() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a bit of doc comment here highlighting that this is triggered explicitly after the document has been updated, and use your use-case as a "for example"? i.e. "Sends a signal that the document has been updated. For example, you can use this signal to retrieve heads
for the document to capture the state after you applied changes." - or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I'm just in the middle of a mad summer dash, so won't get to it today, but will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jesse - I'll go ahead and merge this and add the docs after.
I think this is all good. I have a slightly uncomfortable feeling about the re-entrancy because we use an To make that concrete, here's pub fn put_in_map(&self, obj: ObjId, key: String, value: ScalarValue) -> Result<(), DocError> {
let obj = am::ObjId::from(obj);
let mut doc = self.0.write().unwrap();
assert_map(&*doc, &obj)?;
doc.put(obj, key, value).map_err(|e| e.into())
} ( You can see here that we panic if we can't obtain the lock (the |
On the Apple platforms side, NSRecursiveLock (the lock in question here) only allows the same thread to pass through the lock, so I think it prevents the bits where we'd get re-entrancy across multiple threads, and with a single thread access we'd be fine - it would come through to the Rust side as serialized calls to the llibrary since they're all coming from a single thread. Other lock implementations here could be very poor, so I'd want to not replace this with a "cheaper lock" down the road, as the different semantics could easily bite us, but this I think is going to be fine - a good choice for the pain point/use-case that Jesse's hitting. |
This gives Document an .objectDidChange publisher, that is fired in sync with and immediately after changes are made to a document. This required two changes:
resolves #178