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

IAsyncDisposable, using statements, and async/await #114

Closed
sharwell opened this issue Jan 28, 2015 · 77 comments
Closed

IAsyncDisposable, using statements, and async/await #114

sharwell opened this issue Jan 28, 2015 · 77 comments

Comments

@sharwell
Copy link
Member

With the introduction of support for await inside of a finally block, I'd like to propose the following extension to the using statement in C#.

The System.IAsyncDisposable interface

public interface IAsyncDisposable : IDisposable
{
  Task DisposeAsync();
}

Modification to the using statement

When a using statement appears inside of an async method, the translation of the using statement is modified to the following.

A using statement of the form

using (ResourceType resource = expression) statement

corresponds to one of four possible expansions. When ResourceType is a non-nullable value type which implements System.IAsyncDisposable, the expansion is

{
    ResourceType resource = expression;
    try {
        statement;
    }
    finally {
        await ((IAsyncDisposable)resource).DisposeAsync();
    }
}

Otherwise, when ResourceType is a non-nullable value type which does not implement System.IAsyncDisposable, the expansion is

{
    ResourceType resource = expression;
    try {
        statement;
    }
    finally {
        ((IDisposable)resource).Dispose();
    }
}

Otherwise, when ResourceType is a nullable value type or a reference type other than dynamic, the expansion is:

{
    ResourceType resource = expression;
    try {
        statement;
    }
    finally {
        if (resource != null) {
            IAsyncDisposable tmp = resource as IAsyncDisposable;
            if (tmp != null) {
                await tmp.DisposeAsync();
            }
            else {
                ((IDisposable)resource).Dispose();
            }
        }
    }
}

Otherwise, when ResourceType is dynamic, the expansion is

{
    ResourceType resource = expression;
    IDisposable d = (IDisposable)resource;
    try {
        statement;
    }
    finally {
        if (d != null) {
            IAsyncDisposable tmp = d as IAsyncDisposable;
            if (tmp != null) {
                await tmp.DisposeAsync();
            }
            else {
                d.Dispose();
            }
        }
    }
}

The IAsyncDisposable interface has no impact on the expansion of using statements which appear in any context other than a method marked with the async modifier.

@ljw1004
Copy link
Contributor

ljw1004 commented Jan 28, 2015

I find it scary that there'll be an implicit "await" in your code (i.e. re-entrancy vulnerability) with no indication of it in your code. Could you tell me your thoughts on a compound keyword?

async using (var x = <expr>)
{
   ...
}

@giggio
Copy link

giggio commented Jan 28, 2015

I like the idea and I prefer it with @ljw1004 addition.
Also, would it be possible to change the synchronization context on which DisposeAsync is called?

@ljw1004
Copy link
Contributor

ljw1004 commented Jan 28, 2015

@giggio, ah, excellent point. That's what sunk the idea when we thought about it a few years ago. The point is that a library would always want

await ((IAsyncDisposable)resource).DisposeAsync().ConfigureAwait(false);

but a user app would always want it without the ConfigureAwait.

@sharwell
Copy link
Member Author

I don't have an answer for you yet, but I accept your challenge 😎

@davkean
Copy link
Member

davkean commented Jan 28, 2015

@terrajobst It would be great to merge this proposal with the one that you did a few years back?

@terrajobst
Copy link
Member

I'll dig it up and see what additional thoughts we had back then but as far as I remember, @sharwell pretty much nailed it already.

@weitzhandler
Copy link
Contributor

Ditto.
I also second #114 (comment).

@sharwell
Copy link
Member Author

I propose an extension method be provided for the type IAsyncDisposable. However, while the use case makes sense, I'm not sure this is possible.

public static T ConfigureDispose<T>(this T disposable, bool continueOnCapturedContext)
    where T : IAsyncDisposable

This would allow you to do the following:

using (ResourceType resource = expression.ConfigureDispose(false))
{
    ...
}

@zahirtezcan
Copy link

I hope that expansions are conceptual but not actual, because AFAIK original "using" statement does not perform boxing to a disposable "struct"

@sharwell
Copy link
Member Author

@zahirtezcan The original post uses language and examples very similar to the actual C# Language Specification. The expansions define behavior; a compiler is free to emit IL code that differs from this source code provided the observable behavior matches the definition. The primary case where this happens today involves avoiding boxing of struct types.

@ljw1004
Copy link
Contributor

ljw1004 commented Apr 23, 2015

In my apps, every method call that might go over the network or disk is one that will fail at some point during normal healthy execution of my program and I have to cope with it. So I wrote two NuGet packages (AsyncStackTraceEx and AsyncMrFlakey) so I can write

Await FooAsync().Log("a")  ' uses a library that gives better Exception.StackTrace in case of failure
Await FooAsync().Flakey()  ' lets me simulate a network/disk failure during my testing

I guess I'd now have to add overloads for .Log and .Flakey which take an IAsyncDisposable, as well as the existing overloads that take Task and Task and IAsyncAction and IAsyncOperation.

@kbirger
Copy link

kbirger commented Apr 23, 2015

I think this is not a pattern that should be promoted. Your code should not typically rely on the result of a dispose method, and your dispose should not be long running. If you need to do lots of work in your dispose, then it's already a red flag. If that's the case, then you should just do a Task.Run() inside the dispose and return immediately.

Microsoft has been extremely apprehensive to support situations where you might implicitly block a method for an indeterminate amount of time.

@GrabYourPitchforks
Copy link
Member

I concur with @kbirger on this. The intent of a Dispose method is to release a resource opportunistically, and resource teardown should almost always be synchronous / short-running. Allowing async disposal seems to encourage the "perform some mandatory non-resource-teardown action at this deterministic time" pattern. In async methods in particular this pattern could lead to a pit of failure because (a) the async callback by definition does not run at a deterministic point and (b) it's not even guaranteed to run at all.

(I'm speaking only for myself, not any team within Microsoft.)

@ljw1004
Copy link
Contributor

ljw1004 commented Apr 25, 2015

@GrabYourPitchforks, I disagree. Think of a network connection with a server. It's reasonable that "disposing" of the connection would involve sending a message to the server to let it release its resources gracefully, and awaiting until the server has either acknowledged receipt of that notification, or torn down its resources.

@kbirger
Copy link

kbirger commented Apr 25, 2015

@ljw1004, in your example, it sounds like there's also a chance that the graceful bit won't happen, which means there could be an exception. This would be highly undesirable behavior. And again... if it's a potentially long-running task, it doesn't belong in a dispose method. If you must put it in a dispose method, then it should be fire-and-forget, which is to say Task.Run().

@ljw1004
Copy link
Contributor

ljw1004 commented Apr 26, 2015

Just putting it out there -- even WCF Client (which you get from AddServiceReference) has "CloseAsync".

@sharwell
Copy link
Member Author

  • Not even FileStream.Dispose() meets the requirement of a short running operation that won't take an indeterminate amount of time.
  • Dispose() was never a guaranteed call. Not even the resource cleanup of CriticalFinalizerObject is guaranteed.
  • This proposal does provide deterministic cleanup, in the sense that you know when your Task completes. It's very different from leaving everything up to the GC.

@kbirger
Copy link

kbirger commented Apr 27, 2015

@ljw1004 - I think these are different. An asynchronous close has its place, and you will always have the ability to create a closing method which returns a task, but that is different from having this done automatically in a using block.

In what way is Dispose() not guaranteed? I'm not sure I grasp what you're saying here.

Another thought I have about the long-running aspect is, it seems that for clean-up code, you would want to be able to set a timeout. It sounds like it would be more elegant to just handle it using try-finally at this point, so that you can correctly handle the control logic for your cleanup.

@int19h
Copy link

int19h commented May 30, 2015

Something that might be of interest, esp. the rationale for various decisions:

https://www.python.org/dev/peps/pep-0492/#asynchronous-context-managers-and-async-with

@jmaine
Copy link

jmaine commented Aug 18, 2015

I agree with using async using here. Makes things clearer.

@RichiCoder1
Copy link

For real life use cases that fall outside what was mentioned, xUnit just added IAsyncFixture to allow for async initialization and disposal of fixture information: xunit/xunit#424

@jskeet
Copy link

jskeet commented Nov 12, 2015

Just because it hasn't been mentioned as far as I've seen... should it take a cancellation token? That would potentially handle @kbirger's timeout aspect, although of course a cancellation token is advisory rather than "stomp on the thread"...

@RichiCoder1
Copy link

Something like?:

public interface IAsyncDisposable : IDisposable
{
    Task DisposeAsync(CancellationToken token = default(CancellationToken));
}

Edit: how would this work when being used inside a theoretical using... Would you just have to be explicit and unroll it yourself?

@ljw1004
Copy link
Contributor

ljw1004 commented Nov 12, 2015

@RichiCoder1 read the suggestion of @sharwell above which could be adapted to solve this elegantly and still use using:

public static T WithCancellation<T>(this T disposable, CancellationToken token)
    where T : IAsyncDisposable

using (var x = Fred().WithCancellation(c)) {
   ...
}

@jnm2
Copy link
Contributor

jnm2 commented Dec 21, 2016

I had no idea that the current documentation says this literally all over:

Provides a mechanism for releasing unmanaged resources.

The primary use of this interface is to release unmanaged resources.

You should implement IDisposable only if your type uses unmanaged resources directly.

It's of course ridiculous. In the IDE, you're given quick actions to implement either the unmanaged IDisposable pattern or the purely managed pattern.

@jnm2
Copy link
Contributor

jnm2 commented Dec 21, 2016

Also, I am of the frame of mind that the unmanaged (IDisposable + finalizer) pattern should never be used for unmanaged resources when you can implement a SafeHandle instead. Example managed Dispose + SafeHandle.

@binki
Copy link

binki commented Jan 26, 2017

@ljw1004 With all the discussion and suggestions here, what is your current thought on what the syntax/rules should be for this proposal? Or is it too early to ask?

@ljw1004
Copy link
Contributor

ljw1004 commented Jan 26, 2017

@binki I'm no longer the champion for this feature (I switched job last fall). In my mind it remains vexing...

I'm interesting in Kotlin continuations: https://github.com/Kotlin/kotlin-coroutines/blob/master/kotlin-coroutines-informal.md#continuation-interface -- these provide a uniform library-defined mechanism for writing any kind of resumable method (e.g. async methods, iterators, async iterators, ...). So someone could write await(...) just as a library method and it would do the same compiler transformation as C# does, or they could write yield(...) as a library method. So they must deal with the same issue as IAsyncDisposable. How do they deal with it?

@binki
Copy link

binki commented Jan 26, 2017

Hmm, Kotlin coroutines defines an extension method with a language-supported suspend keyword (I’m confused, I can’t see that keyword defined in the docs—it’s not somehow defined by the library, is it? I don’t think Kotlin is quite like Haxe which has a rich macro syntax/codgen support enabling you to try to imitate async/await transformations without even modifying the compiler). suspend seems to be like async/await without await. Somehow calling suspend functions from other ones gives the called function the ability to suspend the caller.

How they would handle using (or use as they call it) is the same as using a C# extension method accepting either Task<T> or T where T : IAsyncDisposable which would be similar in nature to the “workaround” I wrote earlier. Unfortunately, C# starts looking ugly when each line is a new lambda call (whereas kotlin makes lambdas look almost like blocks—example of their synchronous using). But maybe this looks OK:

// This would call the Task<T> overload of UsingAsync(). There should *not*
// be parens around the await.
await GetResourceAsync().UsingAsync(async resource => {
    await resource.DoSomethingAsync();
});

…where UsingAsync would use try{…}finally{…} to call IAsyncDisposable.DisposeAsync() at the appropriate time. It requires the developer to be disciplined about keeping the instantiation/acquisition of resources and call to UsingAsync() close to each other/on the same line.

Maybe if this pattern could be promoted to the BCL, that would be enough. No need for new confusing syntax since that was hard to agree upon?

@jnm2
Copy link
Contributor

jnm2 commented Jan 26, 2017

I do not want a lambda. You get capture issues and warnings, it looks ugly, it's hard to type, it's additional overhead that the try...finally await DisposeAsync() pattern does not have

@binki
Copy link

binki commented Jan 26, 2017

@jnm2 Though, regarding capture issues, wouldn’t you have no issues beyond the same issues you have with async anyway? I.e., is there actually something you can do inside try{…} in an async function but not in a lambda? Also, I am not sure, but wouldn’t the lambda’s capture reuse the same locals-storing object that C# is already creating for the outer async function and only create an extra object for its own locals? I do expect using the lambda to add a bit of overhead that could be avoided with new syntax, though, so I guess new syntax is preferred after all.

@dmitriyse
Copy link

dmitriyse commented Feb 8, 2017

Please consider another design

    /// <summary>
    /// Asynchronously disposable.
    /// </summary>
    public interface IAsyncDisposable
    {
        /// <summary>
        /// Dispose task. Returns valid dispose task even <see cref="StartDispose"/> was not called yet. <br/>
        /// When something goes wrong and problem cannot be handled - task should be completed with unprocessable exception to hint
        /// application crash.
        /// </summary>
        Task DisposeTask { get; }

        /// <summary>
        /// Initiates async disposing, allowed to be called multiple times. Should never <see langword="throw"/> an exception.
        /// </summary>
        void StartDispose();
    }

I found this design many times more suitable in actual framework building. But possibly both design should coexist (for different purposes).

@dmitriyse
Copy link

One big change to original proposal is the ability to have valid "Task" that can be used for continuations and regardless of dispose started or not.

@binki
Copy link

binki commented Mar 2, 2017

@dmitriyse A class desiring such “only start the disposable once” behavior could just return the same Task in subsequent calls to IAsyncDisposable.DisposeAsync(). I can sort of see how it might be easier to consume what you wrote, but I think the original proposal makes some good decisions:

  1. An interface with just Task DisposeAsync(); is simpler to implement. It only has one member.
  2. The original proposal is closer to the existing IDisposable.Dispose() which developers are already familiar with. The synchronous corollary has the same issues that are solved by your suggestion, but in practice developers either fix consuming code to only ever call Dispose() once (especially when consuming BCL classes) or they make their Dispose() implementations idempotent. Introducing a new interface where the “only” difference is that it’s asynchronous instead of synchronous makes it easier for existing C# developers to adapt to. And if you want to make your implementation idempotent, you’re always free to just return the same Task in subsequent calls.

@dmitriyse
Copy link

dmitriyse commented Mar 2, 2017

Hi! Yes, returning the same Task from AsyncDispose is a good and possible idea. And I also share your reasons. But one case is not covered by Task DisposeAsync(). You cannot get Task and not query disposing.
So task in my suggestion works as event. You can subscribe on this task even no Dispose been started.
In the original proposal:
"Task DisposeAsync()" method is only way to get Task. So you will always start to dispose before receive awaitable Task. My library hardly requires behavior when one consumer subscrubes on a dispose Task, and other consumer firing async dispose.

@dmitriyse
Copy link

dmitriyse commented Mar 2, 2017

I think requirement to have "dispose finished" Task-event (while don't do dispose) is not so rare . And if IAsyncDisposable will not support this case than some third party libraries will add additional contract IAsyncDisposableEx "with blackjack and hookers". Possibly it;s a reasonable solution to have IAsyncDisposableEx inherited from IAsyncDisposable with DisposedTask property, because "async using" implemented for IAsyncDisposable will also perfectly work with IAsyncDisposableEx.
So my suggestion should be transformed to:

    /// <summary>
    /// Asynchronously disposable with additional features.
    /// </summary>
    public interface IAsyncDisposableEx: IAsyncDisposable
    {
        /// <summary>
        /// Dispose task. Returns valid dispose task even <see cref="AsyncDispose"/> was not called yet. <br/>
       /// The same task will be always returned from IAsyncDisposable.AsyncDispose() method (in the future calls).<br/>
        /// When something goes wrong and problem cannot be handled - task should be completed with unprocessable exception to hint
        /// application crash.
        /// </summary>
        Task DisposedTask { get; }
    }

@dmitriyse
Copy link

Please also consider to support this scenario dotnet/csharplang#216

Possible way is:

public interface ICancellableAsync: IDisposableAsync 
{ 
    void OnException (Exception ex);
}

@fredericDelaporte
Copy link

Such a feature could be great for inherently IO bound dispose like the TransactionScope disposal too. (Of course it would require enabling async operations in System.Transactions, which currently has no support for it.)

Otherwise ressources which requires IO bound cleanup should all provide some CloseAsync for allowing to await them in nominal cases (when no exceptions have been raised) and have the Dispose freed of those IO bound operation in those cases. The disposal would still perform those operations if required (exception prior to the CloseAync await).

@sjb-sjb
Copy link

sjb-sjb commented Oct 22, 2017

The discussion here about using is a good example of one of the many thorny issues around how to evolve the c# language in view of the new async / await paradigm. Recently I opened a similar discussion about asynchrony and lock (which so far has not gained much traction). There was also a comment made elsewhere that iteration with await inside of yield can lead to similar issues as would occur if await were allowed inside of lock. Also if we look at the discussion above we can see a similar questions starting to arise for try / finally, such as how should finally deal with tasks that are being awaited in the try block, similar to the question of how using should deal with async operations.

Overall, the control structures of c# were designed at a time when the normal programming paradigm was synchronous rather than asynchronous. And when asynchrony did occur, it was usually associated with threads rather than Tasks as we have today.

It seems possible to me that we may need await versions of most of the major flow-of-control structures in the language. So we might have await using (...) {...} and await lock (...) {...} and try { ... } await catch (...) {...} await finally {...} and perhaps even await if (...) {...}. The question of how await should interact with foreach may be already answered by the C#8 proposal for async enumerables.

It's easy for me to suggest, but I would like to call on @MadsTorgersen and the rest of the MS team to step back and give this some comprehensive thought (if it is not already being done).

@mlehmk
Copy link

mlehmk commented May 24, 2018

There is actually no need to asynchronously dispose and object. The state of an object can always be invalidated synchronously and cleanup can occur on the threadpool.
Example for disposable pattern:

protected virtual void Dispose(bool disposing)
{
	if (disposing)
	{
		if(!_disposed)
		{
			_disposed = true;
			Task.Run(async () =>
			{
				// Do asynchronous cleanup here
				await ...
			}
		}
	}
}

Why this works is a contract, that Dispose must not throw an exception.

@davkean
Copy link
Member

davkean commented May 24, 2018

@mlehmk That only works if Dispose has no observable behavior.

Pretend you implement that logic in FileStream that you've opened for exclusive access to a file, that you want to delete that immediately afterwards after disposing the FileStream. How would you know when you can delete the file?

@jinujoseph jinujoseph added this to the Unknown milestone May 24, 2018
@sjb-sjb
Copy link

sjb-sjb commented Jun 11, 2018

Comment: since this thread was opened a couple of years ago, I have been using (ha ha) the following interface: IAsyncShutdown { Task ShutdownAsync();}. I designed my program so that when the service provider scope exits, it will shut down and then dispose all the services.

When I did this, I eventually found that I needed a ShuttingDown event in the IAsyncShutdown interface taking HandledEventArgs. The reason is that the program state may not always be suitable for the execution of the ShutdownAsync implemented by the service. For example, in some program states one may want to abandon an operation rather than complete it, or vice versa.

@jcouv
Copy link
Member

jcouv commented Jun 30, 2018

This is a language design issue, so should be moved to csharplang. Since it is already tracked as part of the async-streams feature (candidate for C# 8.0, prototyped in async-streams branch), I'll go ahead and close the present issue. Thanks

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

No branches or pull requests