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

PhysicalFileProvider and PhysicalFilesWatcher should implement IDisposable correctly #41918

Merged
merged 2 commits into from
Sep 11, 2020

Conversation

Marusyk
Copy link
Member

@Marusyk Marusyk commented Sep 5, 2020

Changes in implementation of the IDisposable pattern for PhysicalFileProvider and PhysicalFilesWatcher

Fixes #41705

Please review
Thank you in advance

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 5, 2020
@ghost
Copy link

ghost commented Sep 6, 2020

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

@jkotas jkotas removed the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 6, 2020
@eerhardt
Copy link
Member

eerhardt commented Sep 8, 2020

Can you also remove the finalizers? Since these classes don't use native resources, they shouldn't have finalizers.

See https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#implement-the-dispose-pattern

Important
It is possible for a base class to only reference managed objects, and implement the dispose pattern. In these cases, a finalizer is unnecessary. A finalizer is only required if you directly reference unmanaged resources.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the Finalizers was a good step, but the extra changes are a step backwards. Can you add them back?

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks for your help here @Marusyk.

@maryamariyan maryamariyan merged commit e716f07 into dotnet:master Sep 11, 2020
public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GC.SuppressFinalize(this); is not necessary on types that don't have finalizers. You removed the finalizer below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was discussed in #41918 (comment)

@Marusyk Marusyk deleted the rmaruyk/41705 branch September 11, 2020 21:05
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
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.

PhysicalFileProvider should implement IDisposable correctly
7 participants