Skip to content

Remove unnecessary Dispose Override from BackgroundWorker#52504

Closed
johnthcall wants to merge 1 commit intodotnet:mainfrom
johnthcall:issue43852
Closed

Remove unnecessary Dispose Override from BackgroundWorker#52504
johnthcall wants to merge 1 commit intodotnet:mainfrom
johnthcall:issue43852

Conversation

@johnthcall
Copy link
Copy Markdown
Contributor

As part of #43852 this Dispose was missing documentation, however as this is the same as the Dispose(boolean) of Component base class removing this should cause the documentation to be inherited just like Dispose() and GetService(Type) is.

@ghost
Copy link
Copy Markdown

ghost commented May 8, 2021

Tagging subscribers to this area: @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

As part of #43852 this Dispose was missing documentation, however as this is the same as the Dispose(boolean) of Component base class removing this should cause the documentation to be inherited just like Dispose() and GetService(Type) is.

Author: johnthcall
Assignees: -
Labels:

area-System.ComponentModel

Milestone: -

@KalleOlaviNiemitalo
Copy link
Copy Markdown

Component.Dispose(bool) removes the component from its container, and raises the Disposed event:

protected virtual void Dispose(bool disposing)
{
if (disposing)
{
lock (this)
{
_site?.Container?.Remove(this);
if (_events != null)
{
((EventHandler?)_events[s_eventDisposed])?.Invoke(this, EventArgs.Empty);
}
}
}
}

BackgroundWorker.Dispose(bool disposing) neither calls base.Dispose(disposing) nor does the same things on its own. If you remove that override, then those things will happen when a BackgroundWorker is disposed. Which may be an improvement but anyway increases the risk from the change.

The empty BackgroundWorker.Dispose(bool) method was changed from virtual to override in dotnet/corefx#12324 when Component was added as a base class. In .NET Framework though, BackgroundWorker overrides neither Dispose() nor Dispose(bool), and thus lets Component.Dispose(bool) do its thing.

So, removing the override makes sense to me. However:

  • Is that a breaking change if an app defines a class based on BackgroundWorker and calls base.Dispose(disposed)?
  • Do the other classes that were changed in the same pull request have Dispose(bool) overrides that prevent Component.Dispose(bool) from working? The missing base.Dispose call was apparently added to FileSystemWatcher in Add missing FileSystemWatcher methods corefx#13656, but it may still be missing from Process.

@johnthcall
Copy link
Copy Markdown
Contributor Author

@KalleOlaviNiemitalo thanks for the catch, I had gone to definition of Component but was incorrectly looking at the Decompiled source where it showed no code in the Dispose method.

@johnthcall johnthcall closed this May 10, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants