Skip to content
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

What about WeakReference<Observer<T>> instead of Observer<T>? #307

Closed
IvanMurzak opened this issue Feb 12, 2025 · 3 comments
Closed

What about WeakReference<Observer<T>> instead of Observer<T>? #307

IvanMurzak opened this issue Feb 12, 2025 · 3 comments

Comments

@IvanMurzak
Copy link

IvanMurzak commented Feb 12, 2025

Classical issue - memory leak because someone forgot to unsubscribe ⚠

Looks like we can resolve the dirty problem in a pretty beautiful way. Instead of forcing a developer to Dispose subscription explicitly, we can use WeakReference under the hood. Which will bring a minor opposite problem. As a result if developer won't store the IDisposable (subscriber) reference, the subscription would "expire".

I believe this "opposite" problem is better because:

  1. it is very intuitive issue - most of developers would resolve easily
  2. the issue happens almost immediately after subscription if developer didn't store a reference, which helps people to realize it

On other hand, if to keep doing as we are doing right now with hard reference, the memory leak problem that we have is extremely unintuitive and badly discoverable. Also it happens much later after the initial subscription which doesn't help.

Current source

public readonly Observer<T> Observer;

Update proposition

- public readonly Observer<T> Observer;
+ public readonly WeakReference<Observer<T>> Observer;

Of course it would require some amount of refactoring, the Observer has around 10 use spots, not too bad.
Please let me know if I am going to crazy, but it seems as "simplification" of existed things in the world of reactive programming to me. I am curious what do you think about this idea?

@neuecc
Copy link
Member

neuecc commented Feb 13, 2025

I don't want to take the WeakReference approach.

  • The performance impact is unknown
  • Since observer references are scattered throughout various places via operators, it's unclear if that alone would be sufficient for proper cleanup

Of course, solving memory leak issues in Rx programming is important.
In R3, we've prepared a comprehensive solution including cleanup from both upstream and downstream, a rich set of Dispose-related tools, and a tracking system to enable explicit resolution wherever possible.
This should be far more effective at preventing leaks compared to standard Rx and other Rx systems.

@IvanMurzak
Copy link
Author

The performance impact is unknown

It should downgrade performance. At least because that is an extra wrapper. Also if there is any list of Observers, it should to shrink time to time when some objects are finalized.

I can do the research and test in a case if I would know the idea at least has a chance to be integrated if everything would work:

  • automatically unsubscribe
  • no false dispose call

Just please let me know @neuecc if this theoretically acceptable change.

Since observer references are scattered throughout various places via operators, it's unclear if that alone would be sufficient for proper cleanup

When everything what points to the instance is also uses WeakReference - it is fine. I am testing the same thing in my Unity-ImageLoader package, works beautifully to release Texture from memory when there is no references left, without explicit Dispose().

@neuecc
Copy link
Member

neuecc commented Feb 14, 2025

In the past, there was active discussion about approaches using WeakReference in WPF, but I didn't view it very positively.
Rather, I found value in Rx providing explicit management ways.
I won't adopt approaches using WeakReference.

@neuecc neuecc closed this as completed Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants