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

Remove unnecessary finalizers #1209

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Rob-Hague
Copy link
Collaborator

All of the finalizers in the library are no-ops, but their existence means that when Dispose (and thus GC.SuppressFinalize) is not called, the objects' lifetimes are extended unnecessarily while they are waiting to be finalized.

All of the finalizers in the library are no-ops, but their existence means that
when Dispose (and thus GC.SuppressFinalize) is not called, the objects' lifetimes
are extended unnecessarily while they are waiting to be finalized.
@WojciechNagorski
Copy link
Collaborator

I could be wrong, but I think that if we call GC.SuppressFinalize(this) inside the Dispose() method it tells GC that it does not need to run the finalizer at all. If so, nothing will change because of this PR. @Rob-Hague What do you think about it?

https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose

@Rob-Hague
Copy link
Collaborator Author

You are correct. The PR is just small clean-up in the case that Dispose is not called.

@WojciechNagorski
Copy link
Collaborator

@drieseng What do you think about this PR.

@drieseng
Copy link
Member

@drieseng What do you think about this PR.

I'm ok with removing the finalizer when there's no added value, but it made sense to keep the finalizer for the few cases I reviewed (BaseClient, SftpFileReader and PasswordConnectionInfo).

@Rob-Hague
Copy link
Collaborator Author

@drieseng What do you think about this PR.

I'm ok with removing the finalizer when there's no added value, but it made sense to keep the finalizer for the few cases I reviewed (BaseClient, SftpFileReader and PasswordConnectionInfo).

None of them make use of any unmanaged resources, so it doesn't make sense for them to have finalizers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants