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

The guidelines really should be updated to point developers in the right direction #8463

Open
BrunoJuchli opened this issue Oct 18, 2018 — with docs.microsoft.com · 19 comments

Comments

Copy link

commented Oct 18, 2018 — with docs.microsoft.com

The guidelines should read:

When working with unmanaged resources:

  1. use an existing SafeHandle if possible

  2. If not possible: subclass SafeHandle to create one that meets your needs. This class should not do anything more than managing unmanaged resources. It should be sealed.

  3. If that's not possible: create your own class which implements IDisposable + Finalizer

3.1 The class should be sealed

3.2 If sealing the class is not possible: add protected void Dispose(bool disposeManagedResources)

99% of .net developers should never ever actually implement the "full blown" dispose pattern.

CA1063 should be updated accordingly.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@mairaw

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

@Maoni0 can you help with this feedback about GC?

@Maoni0

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

sounds reasonable to me. but this is more a BCL issue. @stephentoub do we already have some sort of guidelines wrt using SafeHandle somewhere?

@stephentoub

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

I don't believe the Framework Design Guidelines that @KrzysztofCwalina previously published covered SafeHandle, other than in comments included in the annotated version. Guidance like the above is reasonable. In particular, I view the discussion related specifically to finalizers: you should really think hard before adding a finalizer to manage unmanaged resources... if you find yourself thinking you need to, you most likely should just use a SafeHandle.

@KrzysztofCwalina

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

@bartonjs and I talked about starting an effort that would serve as an "errata"/update to the FDGs. @bartonjs, maybe this is a good candidate for the first update topic?

@bartonjs

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

FDG2e has a pretty solid opinion here:

  • AVOID making types finalizable.
    • (Extend SafeHandle if you're wrapping a native resource)

9.4.2 (Finalizable Types)

@mavasani

This comment has been minimized.

Copy link

commented Jan 2, 2019

Tagging @sharwell @gpetrou @duracellko as we hit this issue in dotnet/roslyn-analyzers#1950

CA1063 FxCop analyzer tried to aggressively flag all derived types overriding the destructor/finalizer and suggested to remove them, which caused CA1063 to fire on the documentation snippet cited here.

As we concluded in dotnet/roslyn-analyzers#1950 and seems to match with the consensus on this issue, we should strongly push customers towards using SafeHandles instead of providing their own finalizer override for cleanup. We have updated the analyzer to be bit more conservative and only flag finalizer override on a Derived type if there is a finalizer override in at least one of its base type in its inheritance chain, as this is almost certainly likely to cause duplicate Dispose invocations.

Can we please update this documentation to not suggest people towards writing finalizers?

@sharwell

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

I disagree with item 3 in the original list. I am not aware of a circumstance where a user needs to write a finalizer. If a user reaches a step where they are writing a finalizer, they have already made a mistake in an earlier step.

New guidance for IDisposable should be written with the following rules:

  1. The legacy guidance for IDisposable, identifiable by a method with the signature Dispose(bool) should only be followed if and only if the following is true:

    1. The type is derived from a type already following the legacy guidance
  2. A type implementing IDisposable should have the following method:

    public virtual void Dispose()
    {
    }
  3. The Dispose() implementation SHOULD NOT be sealed unless the containing type is also sealed

  4. The Dispose() implementation SHOULD NOT call GC.SuppressFinalize(this) (the method is always unnecessary as the type does not have a finalizer)

  5. The type MUST NOT override object.Finalize

@stephentoub

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

I disagree with item 3 in the original list. I am not aware of a circumstance where a user needs to write a finalizer. If a user reaches a step where they are writing a finalizer, they have already made a mistake in an earlier step.

There are certainly times when custom finalizers are valuable; it's just the 1% case.

@bartonjs

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

@sharwell Most of that guidance is in direct disagreement with the established rules (and guidance published in Framework Design Guidelines).

If nothing else, doing a 180 on the guidance would make for a very hard universe to understand, so I don't see us rewriting them as you suggested.

@BrunoJuchli

This comment has been minimized.

Copy link
Author

commented Jan 3, 2019

@sharwell @stephentoub

Quoting @stephentoub

There are certainly times when custom finalizers are valuable; it's just the 1% case.

Which would be covered by item 2 on the original list (subclassing SafeHandle) (#8463 (comment)):

  1. use an existing SafeHandle if possible
  2. If not possible: subclass SafeHandle to create one that meets your needs. This class should not do anything more than managing unmanaged resources. It should be sealed.

Now, I personally can't fathom a case where it be necessary and sensible to have a finalizer outside of a SafeHandle. But maybe that's my lack of knowledge/imagination, but still, we'd be in agreement it's a very rare case, right? So:

  • Does it make sense to "blow up" documentation just for a very rare case?
  • Since documentation is now much more "alive" would it be an option to omit the rare case until someone actually comes up with that case and can reason why it's needed? At that time we might better know how to deal with this (how to properly write documentation). For now, this would make it simpler to progress here.
@BrunoJuchli

This comment has been minimized.

Copy link
Author

commented Jan 3, 2019

@bartonjs

@sharwell Most of that guidance is in direct disagreement with the established rules (and guidance published in Framework Design Guidelines).

If nothing else, doing a 180 on the guidance would make for a very hard universe to understand, so I don't see us rewriting them as you suggested.

Could you elaborate on that? Personally I find it a very hard universe to understand where documentation is illogical and pointing me in the wrong direction (and, as a result, over the years I've had repeated discussions about this specific topic with many a co worker). Are you sure the universe is easier to understand when documentation is "wrong" as when it's "right" but it's more complicated?


I believe, though, that it's necessary to make the change and it's reasoning "public", so that this "universe" is not harder to understand than necessary.
Focus should be given to the new guidance but the old guidance needs to be mentioned.

How about the following documentation structure:

Pre-amble:

(put this in a box...)
The recommended implementation of IDisposable has been significantly changed in 2019. It is now recommended to handle unmanaged resource cleanup solely in SafeHandles. See [Change History].

Actual Documentation:

(doesn't need a header...)
... written as if the "legacy" documentation didn't exist.

How to handle Legacy code:

The legacy guidance (see [Change History]) for IDisposable, identifiable by a method with the signature Dispose(bool) should only be followed if and only if the following is true:

The type is derived from a type already following the legacy guidance

... Insert Legacy Guidance here...

Change History:

...reason why documentation has changed and how it was changed. Not a full blown git revision list but really just why and how the guideline was changed. Add a link to [How to handle Legacy code]

@sharwell

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

Most of that guidance is in direct disagreement with the established rules (and guidance published in Framework Design Guidelines).

@bartonjs I agree, and I've been an ardent supporter of the established rules for more than a decade. However, there are two issues that came to light over the past two years which eventually resulted in me changing my mind:

  1. No one outside of @stephentoub should be writing finalizers (he works on the core runtime where finalization support underneath APIs like SafeHandle is implemented). It has been argued that additional exceptions to this rule exist, but they are so uncommon that the guidance does not need to account for their existence. I can think of only one case over the past decade where it was used for a purpose, though that case occurred many years ago and I'm not sure it would stand up to scrutiny today.
  2. Few people understand the current rules. It wasn't until I observed relatively widespread incorrect application of the rules in dotnet/roslyn (a place where I would assume the rules would be readily understood and followed) that I realized how difficult it is for users to understand the rules.

The updated rules would be a dramatic simplification which focuses on the only recommended implementation strategy (managed resources), eliminating consideration for a scenario which users should not encounter (finalizers). As a special case, they allow for the ongoing existence of the legacy pattern without introducing API incompatibilities.

@bartonjs

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

I write finalizers all the time, they're useful in tracing object lifetimes. They don't release, but they do get written...

The current rule bullets from FDG:

9.4 (Dispose Pattern)

  • DO implement the Basic Dispose Patten on types containing instances of disposable types
    • bartonjs summary: If you hold a disposable thing, be disposable.
  • DO implement the Basic Dispose Pattern and provide a finalizer on types holding resources that need to be freed explicitly and do not [themselves] have finalizers.
    • bartonjs summary: If you've done the P/Invoke to create a resource, it's your job to free it. But you should have just made a SafeHandle for that resource and then this doesn't apply (because the resource is finalized already).
  • CONSIDER implementing the Basic Dispose Pattern on classes that themselves don't hold unmanaged resources or disposable objects but are likely to have subtypes that do.
    • e.g. Stream

9.4.1 (Basic Dispose Pattern)

  • DO declare a protected virtual void Dispose(bool disposing) method to centralize all logic related to releasing unmanaged resources.
    • bartonjs: Since Dispose has grown in scope since the book was written "unmanaged" doesn't really belong there anymore.
  • DO implement the IDisposable interface by simply calling Dispose(true) followed by GC.SuppressFinalize(this).
  • DO NOT make the parameterless Dispose method virtual.
  • DO NOT declare any [other overloads of Dispose].
  • DO allow the Dispose(bool) method to be called more than once. The method might choose to do nothing after the first call.
  • AVOID throwing an exception from within Dispose(bool)...
  • DO throw an ObjectDisposedException from any member that cannot be used after the object has been disposed of.
  • CONSIDER providing method Close(), in addition to the Dispose(), if close is standard terminology in the area.

9.4.2 (Finalizable Types)

  • AVOID making types finalizable.
    • bartonjs: Goes on to say "Prefer using resource wrappers such as SafeHandle..."
  • DO NOT make value types finalizable.
  • DO make a type finalizable if the type is responsible for releasing an unmanaged resource that does not have its own finalizer.
    • bartonjs: This is "if you're inventing an alternative to SafeHandle". It says that your Finalize method should simply be Dispose(false).
  • DO implement the Basic Dispose Pattern on every finalizable type...
  • DO NOT access finalizable objects in the finalizer code path...
  • DO make your Finalize method protected.
    • bartonjs: C# doesn't allow the option, it just does it.
  • DO NOT let exceptions escape from the finalizer logic, [unless you want the process to terminate].
  • CONSIDER creating and using a critical finalizable object...

Yeah, it's a lot of bullets, but it makes sense with the step by step explanations. A simpler form would be:

  • On a type that directly implements IDIsposable:

    • public void Dispose() { Dispose(true); GC.SuppressFinalize(this); }
    • protected virtual void Dispose(bool disposing) { ... }
  • On a type that extends an IDisposable type, when needed:
    *

protected override void Dispose(bool disposing)
{
    if (disposing)
    {
        _disposableField?.Dispose();
        // other "live graph" operations
    }

    // If there are any struct fields representing resources that need to be released, do so here.
    // Make sure to clean up state so you don't try to release it twice.
    //
    // If you followed the rest of the guidelines you never have code here,
    // because it was a SafeHandle released in the above if.

    base.Dispose(disposing);
}
  • Don't produce new finalizable types
  • Exemplum, hic est SafeHandle faciendi.
  • If you do, implement the finalizer as Dispose(false)

I'm not saying that the current docs are great (or necessarily even say what we want them to say); just that I don't think that changing the guidelines is justified:

  • The current design handles both "simple dispose" and finalizable type chains.
  • "Consistency is the key characteristic of a well-designed framework. It is one of the most important factors affecting productivity. ... Consistency is probably the main theme of this book." (Framework Design Guidelines (2nd edition), p6)
  • Change is terrible, unless it's great (and I don't think that this change is "great")
    • Introducing a split like "for types created before 2019 use this Dispose pattern, and for types created in or after 2019 use this other Dispose pattern" has to have the same level of "wow" to it that async/await have over IAsyncResult/Begin/End.

Also, guidance would still have to be written for the 1% case of a new public finalizable type. It would have to explain how Finalize and Dispose interact, and how to properly handle that for unsealed finalizable types.

If we were making the framework anew, I'd probably prefer the simplified alternative.


My main position, I guess, is that @BrunoJuchli's original description is the right thing to do... to update the documentation to say:

  • If a SafeHandle already exists for your needs, use it.
  • If one doesn't, here's how you make your own SafeHandle type.
  • Are you still here? Are you sure you need a finalizer? See previous points... Ugh. Okay. Here's how to do it "right" (given that doing it right is to not do it, but to instead use SafeHandle).
@stephentoub

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

Which would be covered by item 2 on the original list (subclassing SafeHandle)

No, they all won't.

Now, I personally can't fathom a case where it be necessary and sensible to have a finalizer outside of a SafeHandle.

Here are just a few examples off the top of my head (I'm sure there are more):

@sharwell

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

I write finalizers all the time, they're useful in tracing object lifetimes

@bartonjs I don't know enough about your application to know why you are tracing lifetimes in this way. However, when (rarely) faced with similar situations in the past, I've always found alternative solutions viable (typically event tracing / profiling, but could also be dependent objects).

Here are just a few examples off the top of my head

@stephentoub Your examples are fascinating, but to me they highlight how far the core framework goes out of the way to ensure users do not need to write their own finalizers. I don't see dotnet/coreclr or dotnet/corefx as the primary audience for the updated guidance.

  • HttpConnection: This API is one of the closest to what could appear in client code. However, the use of a finalizer appears to be largely due to a combination of optimizing both for maximum performance (avoid unnecessary allocations) and hardening against misuse by unknown downstream clients. If a non-framework developer was implementing a similar feature, it's unlikely that avoiding the object allocation would result in observable performance overhead (similar to e.g. TaskExceptionHolder, but derived from CriticalHandle). In a case where it was observable (and mattered), the client would likely be interested in finding a way to avoid the finalizer cost altogether, instead focusing on ensuring all relevant objects are disposed at the correct times.
  • Timer: TimerHolder could arguably be derived from CriticalHandle. Maybe there's a reason the core framework cannot do this, but for users facing a similar situation it would be a viable option.
  • RemoteExecutorTestBase: This code could likely be improved by tracking open handles in the test class, and verifying either in Dispose or IAsyncLifetime.DisposeAsync that all handles have been closed. Unlike the finalization approach, this would result in deterministic validation of cleanup during the test lifetime, and failures would appear as test failures for the test that failed to clean up. This approach would not require (or benefit from) the use of a finalizer.
@stephentoub

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

TimerHolder could arguably be derived from CriticalHandle

It doesn't have an IntPtr handle. So this is just another way of writing a finalizer (a harder way), because you have to override everything to do with that handle, instead of just writing a finalizer. Why?

This code could likely be improved by tracking open handles in the test class

That then requires way more boilerplate, doesn't work well with nesting (which these do), etc. Your suggesting a workaround for something that doesn't need a workaround.

HttpConnection

You're very focused on deriving from CriticalHandle. It represents a handle! That's not relevant here at all. There is no IntPtr handle. Any such handle we create would be completely fabricated, and we'd doing it purely to use a different syntax for authoring a finalizer.

but to me they highlight how far the core framework goes out of the way to ensure users do not need to write their own finalizers

I disagree. These aren't about ensuring users don't need to write finalizers, these are because a class was needed that needed such functionality.

@sharwell

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

My claim is the current guidance for disposable objects is a large cognitive overhead, and that the cognitive overhead is substantially reduced by disallowing a specific construct (finalizers). I further claim that this is reasonable because both of the following are true:

  1. Cases where custom finalizers are needed because they serve a purpose are exceedingly rare.
  2. Even within the set of cases mentioned by (1), the subset of those cases for which CriticalHandle cannot be leveraged to avoid the use of a custom finalizer are exceedingly rare.

Simplification of this pattern would allow average developers to be more successful (make fewer mistakes) by avoiding a known problem area.

@stephentoub

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

We should not be "disallowing" finalizers; they are a valuable and viable tool. And I agree with @bartonjs on the guidance:

  • Have native resources you need to free? Use SafeHandle if at all possible, starting with built-in ones and writing your own if they don't suffice.
  • Have to do some other kind of clean-up that's not as clean as calling some function to close/clear an IntPtr? Here's how to use finalizers correctly.

I don't believe the need for custom finalizers is "exceedingly" rare. A large majority of the cases are nicely handled by SafeHandle, and for everything else, there are finalizers. Saying that CriticalHandle should be used instead in such cases (where there is no handle) makes zero sense to me: you're still writing a finalizer, you're just doing it in a way that's more convoluted and more expensive.

@BrunoJuchli

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

@stephentoub

I don't believe the need for custom finalizers is "exceedingly" rare.

I tried searching github for C# code containing ": IDisposable" and "~". Unfortunately the search ignores both ":" and "~". So it's a bit hard to get unbiased data.
So all i've got is annecdotal data: In 10 years of development I encountered exactly one finalizer implementation not part of the .Net Framework. It was in MahApps.Metro and it was faulty - the only reason it got relevant for me (fixing a bug...).
Most of my work colleagues during that years would not have been able to detect and understand the issue. They've never written a finalizer and never needed one. They have, however, been implementing IDisposable many, many times.
Almost every time the following IDisposable implementation sufficed:

public void Dispose()
{
    this.something.Dispose();
}

Sometimes this was sensible (but mostly would not be implemented...):

public void Dispose()
{
    this.something.Dispose();
    this.isDisposed = true;
}

public void Foo()
{
    if (this.isDisposed)
    {
        throw new ObjectDisposedException(this.ToString());
    }
}

And in rare cases double disposal needed to be handled (i.E. perform cleanup only the first time Dispose is called, the second time "do nothing").

Actually, quite a lot of "our" IDisposable implementations might be considered a "misuse". They only exist to be able to write:

using(var foo = new Foo())
{
    foo....
}

but do not represent an actual resource which requires cleanup.

Now, quite obviously I've never been tasked with implementing a framework or a library...


I do believe the need for custom finalizers to be very rare.
I'm pretty convinced that the majority of dev's is not creating librarys and frameworks. I even do believe the suggested "basic dispose pattern" is unnecessary complex. In most cases you can keep things extremely simple by making the class sealed.


Now, I also believe it's bad to hide existing complexity from users too much. I believe users should be guided to the "simple path" but, if necessary, should be educated about the complex use cases.
I also understand the need for documentation to cover the "legacy" guidance - as implementations will be common for many years to come.
I also believe documentation should not have the same guidance (focus, preferrence) as the Framework design guidelines. Developers are commonly not creating frameworks.

Therefore, personally, I'm back at my original suggestion: #8463 (comment)

I'd also like documentation to contain a proper explanation for the need of non-throwing, safe, double-disposal and the "basic dispose pattern" and proper extensibility in case of non-sealed IDisposable implementations. And of course: how to actually use SafeHandles and how to extend SafeHandle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.