The dispose pattern docs should not use an IntPtr.Zero handle

[antonfirsov]

Describe the issue or suggestion

The following doc was likely created in .NET Framework times, where the BaseClassWithSafeHandle code example creates and closes an IntPTr.Zero handle, which is NOP on Windows: https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#implement-the-dispose-pattern

The problem is that on Unix FD 0 corresponds to stdnin which means that disposing BaseClassWithSafeHandle will close the standard input. What's even worse, that it will free up FD 0 for usage in other places, eg. Socket meaning that creating and disposing BaseClassWithSafeHandle several times may close the handle under an existing socket leading to unwanted side effects.

There were cases where users left the dummy code from the sample in their codebase, eventually leading to hard-to-diagnose production errors, see dotnet/runtime#56750, dotnet/runtime#64305, fabian-blum/AspNetCore.Identity.LiteDB#14.

We should change this example.

---

Document Details

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

• ID: 572ef6c6-5066-4a14-76a0-c3fb4b62e604
• Version Independent ID: 9c754da1-c234-fb12-580b-0fcf9ffe2c20
• Platform ID: de77d8c7-6af8-0b64-9357-30e46ab49cbd
• Content: Implement a Dispose method - .NET
• Content Source: docs/standard/garbage-collection/implementing-dispose.md
• Service: dotnet-fundamentals
• GitHub Login: @gewarren
• Microsoft Alias: gewarren

[antonfirsov]

@IEvangelist @gewarren I want to fix this since it keeps biting people, but I need your input before I go ahead. Copying the whole sample here for convenience:

public class BaseClassWithSafeHandle : IDisposable
{
    private bool _disposedValue;
    private SafeHandle? _safeHandle = new SafeFileHandle(IntPtr.Zero, true);

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!_disposedValue)
        {
            if (disposing)
            {
                _safeHandle?.Dispose();
                _safeHandle = null;
            }

            _disposedValue = true;
        }
    }
}

As explained above, new SafeFileHandle(IntPtr.Zero, true) should be removed from the code sample, since it leads to catastrophic consequences if accidentally left in the code and released to production on Unix systems. I could do a simple bandaid fix by replacing the dummy SafeHandle with a more innocent one, eg.

- private SafeHandle? _safeHandle = new SafeFileHandle(IntPtr.Zero, true);
+ private SafeHandle? _safeHandle = File.OpenHandle(Path.GetTempFileName(), FileMode.CreateNew)

but I would be unhappy with the result since it would be unclear why are we even showing safe handles instead of demonstrating higher-level API-s (eg. FileStream) and because the example doesn't really demonstrate the semantics of disposing: false. It's the right thing that we want to discourage people from implementing manual resource management and finalizers, but the unsafe resource management is the whole point why Dispose pattern exists and IMO there are better ways to guide people towards safer practices than simply not demonstrating the unsafe ones.

My plan to address this would be the following: (1) change BaseClassWithSafeHandle and BaseClassWithFinalizer so they demonstrate the pattern via an actual unmanaged resource for, but leave a big ⚠ [!WARNING] that such manual resource management should be avoided with the exception of advanced low-level use-cases. (2) Add +1 paragraph to show how to avoid dealing with finalizers and handles by wrapping the resource into a SafeHandle.

Any thoughts? Would you be fine if I PR this change?

[gewarren]

@antonfirsov I'm good with that. Thank you!

[IEvangelist]

This looks great and I love the idea of addressing these issues. Let's see if @sharwell and @bartonjs have any concerns, as they're both key stakeholders with this piece of content.

[bartonjs]

File.OpenHandle is only .NET 6+, so the sample wouldn't be viable for .NET Standard or .NET Framework TFMs.

Changing it to new SafeFileHandle(new IntPtr(-1), true) seems like a more generalized solution to me.

[antonfirsov]

@bartonjs my suggestion in #44201 (comment) is about actually demonstarting Dispose(false) with an unmanaged resource, then demonstrating alternative way with a SafeHandle implementation. I would use Marshal.AllocHGlobal() for this purpose.

Thoughts?

[bartonjs]

That'd be fine by me. It'll make the SafeHandle/Dispose sample bigger since it'll need to define its own version of SafeAllocHandle (e.g.https://source.dot.net/#Microsoft.AspNetCore.Cryptography.Internal/SafeHandles/LocalAllocHandle.cs,21); but since it won't require "new" API the sample is both informative and applies to all TFMs.