Mute the `InvalidOperationException` in `~DisposableCallback`

mrpmorris / Fluxor

Fluxor is a zero boilerplate Flux/Redux library for Microsoft .NET and Blazor.

MIT License

1.23k stars 139 forks source link

Mute the `InvalidOperationException` in `~DisposableCallback` #335

Closed hillin closed 1 year ago

hillin commented 2 years ago

In the finalizer of DisposableCallback, an exception will be thrown if the object was not disposed:

https://github.com/mrpmorris/Fluxor/blob/09ff8ac96ec524a071a7128b71d0e78e734b5762/Source/Lib/Fluxor/DisposableCallback.cs#L70-L79

While this do help exposing bugs, it also come with a great price: if such scenario occurs in a Blazor server service, the entire service will crash. In our case this kind of punishment merely matches the severity of the flaw in our code: it's triggered by a minor component which accidentally implemented IAsyncDisposable but forgot to call Dispose in it because Blazor favors AsyncDispose over Dispose; the effort took us to track down the issue was not cheap either.

I think we should have a way to mute the exception, at least in production environment.

mrpmorris commented 2 years ago

Do you have any suggestions for how to implement that?

hillin commented 2 years ago

Some thoughts:

Add a static property in DisposableCallback to control whether this exception should be thrown;
Optionally, somewhere in the Fluxor.Blazor.Web project, maybe in the StoreInitializer, set the above-mentioned property based on the current ASP.NET environment - preferably disable in production and staging environments, enable in others;
Provide more helpful information in the exception thrown, e.g. the type of the object which was not disposed. This can be an optional parameter passed to the StateSubscriber.Subscribe method.

mrpmorris commented 2 years ago

I can't tie it into a specific asp.net environment because it might be running in a console app, MAUI app, or even WinForms.

hillin commented 2 years ago

That's why I said it could be set in the Fluxor.Blazor.Web project, which is definitely ASP.NET dependent. Anyways that's not the most important thing here.

mrpmorris commented 2 years ago

I'm not sure about this.

If I disable it then you'll get unpredictable errors / memory leaks that are difficult to track down over time. If I leave it in then it throws a very prominent error with instructions for what to look for.

I could update the docs in the link to mention IAsyncDisposable, or perhaps implement IAsyncDisposable instead of IDisposable.