Skip to content

Conversation

@gewarren
Copy link
Contributor

@gewarren gewarren commented Oct 7, 2022

Fixes #8333.

Notes that Dispose(Boolean) is only called by Finalize if it's been overridden.

@ghost
Copy link

ghost commented Oct 7, 2022

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

Issue Details

Fixes #8333.

Notes that Dispose(Boolean) is only called by Finalize if it's been overridden.

Author: gewarren
Assignees: gewarren
Labels:

area-System.Net

Milestone: -

@gewarren gewarren added the area-Meta Concerns something that extends across runtime area boundaries, for example, IDisposable. label Oct 7, 2022
@opbld30

This comment was marked as outdated.

@opbld30

This comment was marked as outdated.

@opbld33
Copy link

opbld33 commented Oct 7, 2022

Learn Build status updates of commit f78cc0d:

✅ Validation status: passed

File Status Preview URL Details
xml/System.CodeDom.Compiler/TempFileCollection.xml ✅Succeeded View
xml/System.ComponentModel.Design.Serialization/SerializationStore.xml ✅Succeeded View
xml/System.ComponentModel.Design/ComponentDesigner.xml ✅Succeeded View
xml/System.ComponentModel.Design/DesignerActionService.xml ✅Succeeded View
xml/System.ComponentModel.Design/DesignerTransaction.xml ✅Succeeded View
xml/System.ComponentModel.Design/DesignSurface.xml ✅Succeeded View
xml/System.ComponentModel.Design/DesignSurfaceManager.xml ✅Succeeded View
xml/System.ComponentModel.Design/InheritanceService.xml ✅Succeeded View
xml/System.ComponentModel.Design/LocalizationExtenderProvider.xml ✅Succeeded View
xml/System.ComponentModel.Design/MenuCommandService.xml ✅Succeeded View
xml/System.ComponentModel.Design/ServiceContainer.xml ✅Succeeded View
xml/System.ComponentModel/Component.xml ✅Succeeded View
xml/System.ComponentModel/Container.xml ✅Succeeded View
xml/System.ComponentModel/MarshalByValueComponent.xml ✅Succeeded View
xml/System.ComponentModel/NestedContainer.xml ✅Succeeded View
xml/System.Data.Common/DataAdapter.xml ✅Succeeded View
xml/System.Data.Common/DbCommandBuilder.xml ✅Succeeded View
xml/System.Diagnostics.Tracing/EventSource.xml ✅Succeeded View
xml/System.Diagnostics/EventLog.xml ✅Succeeded View
xml/System.Diagnostics/TraceListener.xml ✅Succeeded View
xml/System.DirectoryServices/DirectoryEntry.xml ✅Succeeded View
xml/System.DirectoryServices/SearchResultCollection.xml ✅Succeeded View
xml/System.Drawing.Text/FontCollection.xml ✅Succeeded View
xml/System.Drawing/Brush.xml ✅Succeeded View
xml/System.EnterpriseServices.Internal/AppDomainHelper.xml ✅Succeeded View

This comment lists only the first 25 files in the pull request.
For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@gewarren gewarren merged commit 116c7ae into dotnet:main Oct 7, 2022
@gewarren gewarren deleted the dispose-wording branch October 7, 2022 21:16
## Remarks
This method is called by the public `Dispose()` method and the <xref:System.Object.Finalize%2A> method. `Dispose()` invokes the protected `Dispose(Boolean)` method with the `disposing` parameter set to `true`. <xref:System.Object.Finalize%2A> invokes `Dispose` with `disposing` set to `false`.
## Remarks
This method is called by the public `Dispose()` method and the <xref:System.Object.Finalize> method, if it has been overridden. `Dispose()` invokes this method with the `disposing` parameter set to `true`. `Finalize` invokes this method with `disposing` set to `false`.
Copy link
Member

Choose a reason for hiding this comment

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

This remark isn't generic boilerplate but is actually the text on the TempFileCollection page. We know whether TempFileCollection implements a finalizer (it does). Seems like it would make sense to specialize this with that knowledge? Same for the rest of the occurrences here where we know whether it does or doesn't implement a finalizer.

Copy link
Contributor

@antonfirsov antonfirsov Oct 12, 2022

Choose a reason for hiding this comment

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

So you mean specializing this general text in both cases? (when finalizer is overridden VS when it's missing)

What if we change the implementation (delete or add a finalizer)? Are we sure we have the processes, the awareness and the capacity to always ensure the necessary documentation change? Also, I would find a specialized text confusing, in case the finalizer is missing. Dispose(false) is never invoked from the built-in hierarchy, that case only makes sense if a subclass overrides the finalizer. We would need to properly explain this in the text.

IMO the boilerplate text is a safer option if we are not sure if we can (or want to) address these concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Meta Concerns something that extends across runtime area boundaries, for example, IDisposable. area-System.Net

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HttpClient.Dispose documentation says it is invoked by Finalize but the code does not support this statement

5 participants