[BUG] Memory leak using ReactiveUI with Blazor Web Assembly

[RyanMelenaNoesis]

Describe the bug
While setting up a simple test app to explore the usage of ReactiveUI with Blazor (client-side web assembly) I ran into what appears to be a memory leak causing excessive garbage collection followed by an application crash with an OutOfMemoryException. Any insight would be appreciated.

Steps To Reproduce

1. Pull down example project here
2. Run project
3. Open browser debug console
4. Notice that after some time the console will begin showing GC messages and the counter will slow down and eventually the page will crash

Expected behavior
Counter will increment without memory leak

Screenshots

Environment

• OS: Windows
• Version: 10
• Device: Dell XPS-15

Additional context
console.log

[open-collective-bot]

Hey @RyanMelenaNoesis 👋,

Thank you for opening an issue. We will get back to you as soon as we can. Also, check out our Open Collective and consider contributing financially.

https://opencollective.com/reactiveui

PS.: We offer priority support for all financial contributors. Don't forget to add priority label once you start contributing 😄

ReactiveUI - Open Collective

An advanced, composable, functional reactive model-view-viewmodel framework for all .NET platforms!

[glennawatson]

I will see if i can early next week get some more information on this issue, so we can track it down to either rxui or rx libraries.

[ronaldvdv]

@glennawatson I faced the same issue. It can be fixed by

• moving the second WhenAnyValue case in OnAfterRender into the if-close
• caching the results of both WhenAnyValue-statements into an CompositeDisposable
• disposing this in Dispose(bool) method

Also, I believe the statement InvokeAsync(StateHasChanged) leads to converting StateHasChanged into a new Action instance for every call, which can be avoided by creating a single Action instance in the constructor for this purpose.

[RyanMelenaNoesis]

@ronaldvdv Thank you for digging into this, do you happen to have a fork with your changes that I could use for testing?

[RLittlesII]

@ronaldvdv Thanks for that assessment. I have noticed some issues with the way this control operates, although I have not encountered this memory leak. I had some code I was working on to clean up some of the state changes logic. I will test those changes against this sample to verify it resolves this issue.

[ronaldvdv]

@RyanMelenaNoesis Nope, I happened to find this close before a deadline so I just added a copy of the {{ReactiveComponentBase}} class with above changes applied to my project and used it as a base for the components that ran out of memory.

@RLittlesII Please let me know if you need my help with a PR in the coming days.

[RLittlesII]

@ronaldvdv We accept PR's, I think there are a few more edge cases that we are finding with the way that Blazor does state changes we are considering. I've had a few other consumers weight in and I want to get all the changes in at once. That said if there is no traction on this soon (end of the week), feel free to submit a PR.

We have been trying to find a happy medium with this component that allows for #2240 to be handled appropriately. Seems our consumers have different needs from the component.

[RLittlesII]

@ronaldvdv I made some changes in the above pull request. I am trying to test it against the sample provided by @RyanMelenaNoesis but my environment seems to dislike my ability to stage the test.

Are you able to verify if this resolves the issues you are facing? If not, please submit a PR with what you think the changes should look like. I believe the above changes are required regardless of whether they fix this exact issue.

[ronaldvdv]

@RLittlesII I'll try and have a look at this in the coming days, thanks for investigating! After a quick first look at the PR, I believe a difference with my solution is that you're not storing and disposing the result of the Subscribe-calls. I think this would cause the PropertyChanged events on the view model to keep a reference to the view. I think this could be a problem.

For example: If component A contains component B and they both use the same view model, changing a property on that view model would cause component A to rerender which may give a new instance for component B. However, the old copy would still receive property change notifications from the same view model instance.

[RLittlesII]

@ronaldvdv That would make sense due to activation/deactivation that we would have to perform some cleanup. I am going to get the sample provided working in my environment and see if I can test a solution.

[b-straub]

I had a more detailed look to your PR #2391.
The way you are using the Publish().RefCount() pattern will not work.

You intention seems to be having two kind of StateHasChanged notifications. One after the assignment of the ViewModel and subsequent ones when any of the reactive ViewModel properties issue a changed notification. With the provided code this will not work, since the second subscription to Observable.FromEvent<PropertyChangedEventHandler will never be called.
You have several ways to fix it, either .Publish.RefCount(2) or .Replay(1).RefCount(). By the way without this fix even your original OutOfMemoryException sample doesn't seem to work.

I have pulled your sample and applied another way of fixing. Please have look.
MemoryLeakBlazor

[RLittlesII]

@b-straub I will be looking at this later this evening. I have an environment where I can test the change, I was having a problem with running the Blazor example provided here. I will follow up tomorrow with more information.