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

[Bug] widen pit of success for dispose pattern implementations and cleanup currently incorrect ones #6305

Open
PureWeen opened this issue May 24, 2019 · 7 comments

Comments

@PureWeen
Copy link
Contributor

commented May 24, 2019

Description

Dispose should follow the following execution path everywhere
https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#implementing-the-dispose-pattern-for-a-derived-class

	protected override void Dispose(bool disposing) => DisposeMe(this, () =>
		{
			if (Control != null)
			{
				Control.Checked -= OnNativeChecked;
				Control.Unchecked -= OnNativeChecked;
			}
		}, disposing, ref _isDisposed, base.Dispose);


		static void DisposeMe(IDisposable disposable, Action disposeAction, bool disposing, ref bool _isdisposed, Action<bool> baseDispose)
		{
			if (_isdisposed)
				return;

			_isdisposed = true;
			if (disposing)
				disposeAction();

			baseDispose(disposing);
		}

Call Dispose(false) from finalizer on base class

https://docs.microsoft.com/en-us/dotnet/api/system.idisposable?view=netframework-4.8#examples

Possible cleanup option

Helper Method

Create a reusable extension method that correlates to the pattern

static void DisposeMe(this IDisposable, Action disposeAction, disposing, ref _isdisposed)

Reasons not to do it this way?
I'm not sure the overhead of closures and lambda functions or if that's even something to consider.

Also I wouldn't just apply this everywhere. We can just apply this to areas that are broken and then as the ball rolls forward just use the reusable method

Analyzer

Setup an analyzer that will check for dispose patterns so that we don't incur extra allocations by using a function

@PureWeen PureWeen added this to To do in v4.1.0 via automation May 24, 2019
@pauldipietro pauldipietro added this to New in Triage May 24, 2019
@hartez

This comment has been minimized.

Copy link
Member

commented May 24, 2019

You're missing the if(disposing){} in your pattern example.

@hartez

This comment has been minimized.

Copy link
Member

commented May 24, 2019

So with your extension method, what would a Dispose(bool disposing) override look like?

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

@hartez updated example

@hartez

This comment has been minimized.

Copy link
Member

commented May 24, 2019

For things that inherit from ViewRenderer we could also add a helpful base method

I'm wary of this enhancement, because then we run into possible issues with the order in which implementors call base.DisposeRenderer(). I think I prefer confining all disposal code to one place.

@hartez

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Reasons not to do it this way?

I think we'd be looking at a little bit of a performance hit, because it has to allocate a closure. And we'd be giving the GC more work to do.

I suppose the question is whether it's worth the extra hit to prevent us from incorrectly implementing Dispose. I feel like we could probably do that with a combination of reviews and maybe a Roslyn analyzer.

@PureWeen PureWeen removed this from New in Triage May 24, 2019
@PureWeen PureWeen changed the title [Bug] Clean up dispose patterns [Bug] widen pit of success for dispose pattern implementations May 24, 2019
@PureWeen PureWeen changed the title [Bug] widen pit of success for dispose pattern implementations [Bug] widen pit of success for dispose pattern implementations and cleanup currently incorrect ones May 24, 2019
@PureWeen

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

I like the Roslyn analyzer. approach
Seems like that should exist already :-) Edited the options and title a bit more

@PureWeen

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

PureWeen added a commit that referenced this issue Jun 13, 2019
a la #6305
samhouts added a commit that referenced this issue Jun 19, 2019
* [Android] Add disposed check to MasterDetailPageRenderer

Co-Authored-By: Felipe Silveira <transis2@users.noreply.github.com>

* Fix dispose pattern

a la #6305

* apply same set of changes to NavigationPageRenderer

fixes #3489
closes #4586
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v4.1.0
  
To do
2 participants
You can’t perform that action at this time.