Closed SebastianStehle closed 10 months ago
Hi, @SebastianStehle!
I agree with the idea, just not sure how to implement them yet.
I'd like a solution that doesn't involve exposing anything different than we have today. This should probably be handled internally by the library objects (for example, saving a reference to the "unsubscribe" subject).
What do you think?
I noticed - by making the mistake in my application - that even such a simple case where you want to subscribe forever is also buggy:
var doc = new Doc();
var map = doc.Map("A");
map.ObserveDeep(() => {
});
This is how I implemented it in C#:
The subscriptions are managed by a subscriber per event. If you have 10 .NET subscriber only one subscription is created to rust. it works similar to Reactive RefCount operator that converts a cold to a hot subscription.
https://github.com/SebastianStehle/ydotnet/blob/improve-output/YDotNet/Document/Doc.cs#L73
The subscriber also catches exceptions, which is super important to avoid any unpredictable problems in rust: https://github.com/SebastianStehle/ydotnet/blob/main/YDotNet/Document/Events/EventPublisher.cs#L35
Furthermore the subscriber adds itself to a event manager that exists per doc, this keeps the subscription alive as long as the document lives: https://github.com/SebastianStehle/ydotnet/blob/main/YDotNet/Document/Events/EventSubscriber.cs#L39
Hi, @SebastianStehle!
About this:
I noticed - by making the mistake in my application - that even such a simple case where you want to subscribe forever is also buggy: (...)
You mean the Delegate
instance will be collected and this will cause a NullReferenceException
when the Rust code tries to invoke the callback, right?
If so, this scenario is already fixed in a different way as I mentioned in this comment. Your solution looks good to me as well, it's just a bit heavier because of the extra code to "intermediate" the callback process.
You mean the Delegate instance will be collected and this will cause a NullReferenceException when the Rust code tries to invoke the callback, right?
If it would be a NullReferenceException it would be great, but it is TargetInvocationException or something similar that cannot be catched.
If so, this scenario is already fixed in a different way as I mentioned in https://github.com/LSViana/ydotnet/pull/44#issuecomment-1762364209. Your solution looks good to me as well, it's just a bit heavier because of the extra code to "intermediate" the callback process.
Yes, your solution was also my first approach, but then you have to store the event subscription somewhere, even if you never unsubscribe. I made this mistake in my application code and then I changed it.
I actually built the subscriber for something else. The idea was to subscribe on the root types when they are created the first time and then use the event to mark on branches as deleted when they are removed from an array or map or so. Then I would have an open subscription anyway and could reuse that for the user subscription. But the array event does not expose the old values (yet?), so it does not work.
Hi, @SebastianStehle!
Now that #55 is merged and the EventSubscription
objects have been changed to IDisposable
instances, can we close this issue?
I have seen that there is the open task to call dispose on the EventSubscription. We could just use the Dispose method to unsubscribe, e.g.
Advantages: