Proposal: Allow Dispose by Convention

[HaloFour]

Copied from dotnet/roslyn#11420

Currently the using statement is tied specifically to implementation of the IDisposable interface. This makes it impossible to use resource-like objects from other assemblies within using statements.

I propose that the compiler allow resolving an accessible void Dispose() method in the case that the type does not implement IDisposable. That would allow for a developer to extend an existing class through the use of an extension method which would contain the logic for "disposing" of that resource:

public class SomeResource {
    public int Id { get; }
}

public static class SomeResourceExtensions {
    public static void Dispose(this SomeResource resource) {
        Console.WriteLine($"Disposing {resource.Id}");
    }
}

static class Program {
    static void Main() {
        using (var resource = new SomeResource() { Id = 123 }) {
            Console.WriteLine($"Using ${resource.Id}");
        }
    }
}

// output:
// Using 123
// Disposing 123

This brings using in line with the conventions also established by foreach, LINQ and await where the compiler can resort to resolving instance members of a specific name/shape rather than require implementation of an interface or base class.

[jnm2]

Depending how popular this becomes, it makes the job much harder when (obj as IDisposable)?.Dispose(); is no longer guaranteed to do all the cleanup that you're responsible for.

For example, I'll often return a struct enumerable and enumerator and awaitable and awaiter. It's also extremely common for me to return an IDisposable simply to scope something (because I can't use instrumentation clauses yet). If I decided to do the same and return a struct disposable, things like IoC containers and all generic cleanup logic I've ever written will fall on their faces. Sure, it'll work in a using statement, but what if I need to refactor and stash it in a field?

If we had BCL methods to handle this for us it'd be terrific. DuckTyping.GetEnumerator<T>(T), DuckTyping.GetAwaiter<T>(T), DuckTyping.Dispose<T>(T).
The only problem is extension methods- at runtime there's no way to know what extension methods might have been in scope at a given call site.

[jnm2]

I would be happy about allowing using to call a struct Dispose instance method if IDisposable.Dispose is mapped to it.

[msfcolombo]

@HaloFour Is there some additional benefit besides avoiding a wrapper class for the non-standard disposable? I mean, you could use using by simply adding the following class:

class SomeResourceDisposable: IDisposable {
    private readonly SomeResource _actual;
    public Dispose() { _actual.Dispose(); /* Will call your extension. */ }
    ... // constructor etc
}

[scottdorman]

On the surface, I'm not a big fan of this. In C# 1.0, the using statement allowed any class which simply implemented a public void Dispose() method to be used. After C# 1.0 shipped, it was decided that this was a bad idea and the next version strictly enforced that the type implement IDisposable.

Keep in mind that the following code

public void DoWork()
{
    using (MyClass myClass = new MyClass())
    {
        myClass.SomeMethod();
    }
}

is roughly translated by the compiler to

public void DoWork()
{
   MyClass myClass = new MyClass();
   try
   {
      myClass.SomeMethod();
    }
    finally
    {
       if (myClass != null)
       {
          IDisposable disposable = (IDisposable)myClass;
          disposable.Dispose();
       }
    }
}

There are already enough abuses of the disposable pattern and the using statement, mostly because of it's convenience for creating scoped blocks, that I think this would add even more abuses and create more problems.

I think what's really needed is a new pattern to support scoped units of work that has nothing to do with resource allocation and disposability.

[jnm2]

@scottdorman I don't think that using using for scopes rather than resource disposal is abuse. People keep calling it abuse but I don't see any reason for that to be the case. I mean, in a real sense, the scope itself is a resource and it's being disposed. The tracking and management around the scope is being shut down, i.e. disposed.

I think what's really needed is a new pattern to support scoped units of work that has nothing to do with resource allocation and disposability.

That would be #85. It adds capabilities that using does not provide.

[scottdorman]

@jnm2

Structs shouldn't implement IDisposable and shouldn't have unmanaged types since they aren't GC'd.

It is an abuse of the pattern because it has nothing to do with resource allocation. Disposability is supposed to be about deallocating resources, both managed and unmanaged. It has nothing to do with a unit of work ending. Consider a simple "cursor changer" class:

public class WaitCursor : IDisposable
{
    public WaitCursor()
    {
        IsWaitCursor = true;
    }

    public void Dispose()
    {
        IsWaitCursor = false;
    }

    public bool IsWaitCursor
    {
        get
        {
            return Application.UseWaitCursor;
        }
        set
        {
            if (Application.UseWaitCursor != value)
            {
                Application.UseWaitCursor = value;
                Cursor.Current = value ? Cursors.WaitCursor : Cursors.Default;
            }
        }
    }
}

This isn't allocating resources at all, it's simply changing the system cursor. It abuses the IDisposable interface to provide a nice "unit of work" style syntax for the calling code.

Yes, #85 proposes a mechanism to do scoped units of work, but presents it in a very narrowly defined usage pattern, namely for instrumentation. It really shouldn't be so narrow/specific but should be a proposal for a scoped unit of work.

[HaloFour]

@scottdorman

I have to agree with @jnm2, and I think that "abuse" is an abused term here. The using statement may have been intended for unmanaged resource allocation, but it provides a very nice syntax for unit-of-work that doesn't imply in any way that it's so narrowly scoped. Not to mention, that's the only syntax provided in the language and people will find a way to make it fit their needs. If anything it demonstrates a gap in the language, one easily fixed simply by not being ideological. Implementing more keywords like instrumented or unit that would have the same basic functionality wouldn't improve the language in any way, even if each flavor worked with a slightly different type shape.

[jnm2]

@scottdorman

Structs shouldn't implement IDisposable and shouldn't have unmanaged types since they aren't GC'd.

Even structs wrapping disposable classes? I really disagree. What about things like CancellationTokenRegistration? I don't see a problem.

I have no problem stretching the meaning of resource to include usage of a different cursor. I don't see any reason that IDisposable has any limitations on how managed or unmanaged, expensive or cheap, I/O- or memory- or CPU-related or side-effecting the resource is. Anything that can be thought of as a registration can be thought of as a resource, including especially the scoped cursor management. We use awaitables just as liberally as we use disposables. I've even taken advantage of duck typing enumerables:

// foreach over every node in a tree
var stack = new StackEnumerator(root);
foreach (var node in stack)
{
    // ...
    stack.Recurse(node.Children);
}

I maintain that it is not abuse; it's using a language feature to its full potential with no pitfalls.

[ufcpp]

As to the convension, I'd like a builder pattern rather than an extension method.
Recently, I wrote an experimental code for reference counting in C#:

https://gist.github.com/ufcpp/1c7977f4f5f7856787f3f2b6a8b13c8e

In this, I want to call x.Release() instead of x.Dispose() at the end of the using statement.

using (var x = new Sample(1, 2).Init())
{
    for (int i = 0; i < 10; i++)
    {
        items.Add(x.Share());
    }
} // Call x.Release() instead of x.Dispose()

[scottdorman]

The using statement may have been intended for unmanaged resource allocation, but it provides a very nice syntax for unit-of-work that doesn't imply in any way that it's so narrowly scoped.

That's actually my point. It's a nice syntax for a unit-of-work pattern and it's the only one available to us in the language today, so we repurpose the code to do something outside of it's intent.

I'll grant you that I'm being ideological here, but I'm doing it because I think it's an important distinction to make between resource allocation/deallocation and transactional/unit of work scopes. It's sort of like saying that "All types which implement a finalizer should be disposable, but not all disposable types should implement a finalizer." A type which implements IDisposable is, by the fact that it's implementing that interface, intended to deallocate resources at the end of it's scope (when the using statement ends). However, a type which implements IUnitOfWork isn't intended to deallocate resources.

Given that statement, I could see IDisposable inheriting from IUnitOfWork. Maybe even see using extended to work with either interface. I think that would also handle @ufcpp's case of wanting to see a Release() (or probably a better name in this context being Complete()) method called instead of calling Dispose() when the scope ends.

@jnm2 The problem is that there is a well defined pattern for how IDisposable.Dispose() should be implemented. Namely, that it should be

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

That absolutely has everything to do with deallocating resources.

As for CancellationTokenRegistration, I actually wasn't aware that it implemented IDisposable. From some of the text in MSDN, I wonder if this started life as a class and was then changed to a struct and the interface never removed. @MadsTorgersen or @gafter, can you shed any light as to why this struct implements IDisposable and what resources it's freeing?

[jnm2]

@scottdorman

That absolutely has everything to do with deallocating resources.

Cleaning up resources such as a scope registration to restore the cursor if you forgot to call Dispose, yes.

All SuppressFinalize is doing there is keeping the finalizer from calling Dispose(false) to clean up unmanaged resources. In Dispose, if disposing is false, you're not allowed to do anything but release unmanaged resources because you're being called by the finalizer. If your class has no unmanaged resources, you have no need to use this pattern because it adds nothing except a no-op with a tiny bit of overhead. On top of this, it's merely a stopgap in case you shoot yourself in the foot by letting a class with unmanaged resources go out of scope without disposing it. There are very few times this is necessary (CancellationTokenSource itself is one such case) and it's almost more appropriate to put a debug assertion in the finalizer that ensures that it never fires.

[vladd]

Isn't this something which is easily solved with Rx's Disposable.Create? If so, this is than a library question rather than a language question.

[HaloFour]

@vladd

Yes, Rx Disposable helpers do provide a simple way to work around this limitation. Although using it does effectively require at least one allocation.

And Rx does at least demonstrate that not all of MS is so zealous about disposables strictly being about unmanaged resources.

[scottdorman]

@jnm2

If your class has no unmanaged resources, you have no need to use this pattern

Given that logic, then Stream should never have followed this pattern because it owns no resources at all. It follows the pattern because it was understood that concrete implementations of Stream would own resources and, therefore, needed a way to ensure it's derived classes followed the pattern properly.

On top of this, it's merely a stopgap in case you shoot yourself in the foot by letting a class with unmanaged resources go out of scope without disposing it.

Are you referring to following the pattern here? The pattern isn't a stopgap. For that matter, neither is implementing a finalizer, if and when it's necessary. (Finalizers, for the most part, aren't actually necessary anymore, but there are still times when they are.)

@HaloFour, @vladd Yes, the Rx Disposable helpers provide a way to help ensure the pattern is followed in much the same way (conceptually at least) as SafeHandle and all of it's derived classes.

And Rx does at least demonstrate that not all of MS is so zealous about disposables strictly being about unmanaged resources.

That's true but, to some extent I think, it's because there is no other way of achieving the design/syntax goals.

[msfcolombo]

@scottdorman

Yes, #85 proposes a mechanism to do scoped units of work, but presents it in a very narrowly defined usage pattern, namely for instrumentation. It really shouldn't be so narrow/specific but should be a proposal for a scoped unit of work.

To my understanding, unit of work and instrumentation are pretty much the same concept. If not, what is missing on #85 for supporting "scoped unit of work"?

[scottdorman]

@msfcolombo Yes, they are pretty much the same concept, but it's presented specifically in the context of instrumentation, to the point of having the with statement only work with an expression which returns an instance of IInstrument. Other than that (or change that to be something less context specific), and I'm good.

[jnm2]

@scottdorman

You can follow the logic to any conclusions you like, but the logic still holds. The dispose finalizer pattern calls Dispose(false), and Dispose(false) has never been permitted to do anything but release unmanaged resources. If your class does not have unmanaged resources, i.e. is not responsible to close an unmanaged handle, it is a no-op. Furthermore, if you use SafeHandles, any other finalizers are redundant because the GC will run them both. Of course base classes must implement the no-op pattern in case a derived class is the sole owner of an unmanaged handle and needs the protected Dispose(bool), but that's poor practice ever since SafeHandles have been available. Lots of this is legacy.

Yes, the dispose finalizer pattern is a stopgap in almost every case. There are extremely few scenarios where it should ever run. In almost every case, you should have disposed deterministically in scope and not allowed the GC to get there first.

That's why I find putting Debug.Assert in my dispose-suppressed finalizers helpful, because one thing is for sure: that code should not be running.

[iam3yal]

@scottdorman

Let me start with "Unit of Work" and "Resources" are pretty abstract terms, in fact I'd say that they are just fancy words for "Process" and "Things" respectively.

The using statement is used beyond deallocation of resources such as one is wrappers around ReaderWriterLockSlim and WriteLockDisposable where it might look like this:

public static class ReaderWriterLockSlimExtensions
{
	public static IDisposable CreateWriteLock(this ReaderWriterLockSlim locker)
	{
		locker.EnterWriteLock();

		return new WriteLockDisposable(locker);
	}
}

public class WriteLockDisposable : IDisposable
{
	private readonly ReaderWriterLockSlim _locker;

	public WriteLockDisposable(ReaderWriterLockSlim locker)
	{
		_locker = locker;
	}

	void IDisposable.Dispose()
	{
		_locker.ExitWriteLock();
	}
}

Now, the instance of the locker that is passed to WriteLockDisposable is static and is never disposed throughout the application, however, I do need to release the lock at some point so I'm wrapping ExitWriteLock and "exploiting" the using statement to do the job for me.

Variations of this pattern is implemented in the framework but as far as I understand by your definition this would be an "abuse" right? because at the end I'm just creating a scope that prevents multiple threads from reading or writing. :)

In my opinion it really depends on the definition of resources and how much you zoom in to say it's invalid or zoom out to say otherwise.

[omariom]

👍
It would allow conveniently Dispose of ref structs.

[Thaina]

If we just make Dispose extension method on a custom delegate. Does this already account defer feature?

// In corefx

public delegate void Disposal();
public static class DisposeExt
{
    public void Dispose(this Disposal disposal) => disposal?.Invoke();
}

// in our code

using () => DoSomething(); // just convert to Disposal

And if we include #1473 This would also be an #85

// In corefx

public delegate void Disposal();
public static class DisposeExt
{
    public void Dispose(this Disposal disposal) => disposal?.Invoke();
}

// in our code

public (object obj,Disposal instrument) GetObjectWithInstrument()
{
    return (a,() => DisposeLogic());
}

using(var (obj,_) = GetObjectWithInstrument()) /// would be great if we could convert (T,Disposal) to just (T)
{
    DoSomethigWithT
} // _.Dispose(); obj.Dispose();

[rolfbjarne]

I believe this was implemented in C# 8: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/using#pattern-based-using

[HaloFour]

Only for ref struct types because they cannot implement interfaces. In all other cases the type is still required to explicitly implement IDisposable.

[rolfbjarne]

Yeah, I realized that after I tried it myself. The docs are incorrect since they say this applies to all types, so I filed an issue: dotnet/docs#31815.