Implemented IAsyncDisposable for WebApplicationFactory

[ShreyasJejurkar]

1. Another fix - Removed unwanted variable was added as part of Stop the host first and then dispose it! #29404

Fixes #29455

[javiercn]

Looks great!

Thanks for taking care of this so quick!

[ShreyasJejurkar]

@javiercn My pleasure!

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[javiercn]

/AzurePipelines run AspNetCore-ci

[pranavkm]

We shouldn't need to add this API. I think it's sufficient to explicitly implement IAsyncDisposable in derived class and call base.

[ShreyasJejurkar]

You mean, I should remove this method?

[javiercn]

Yes

[ShreyasJejurkar]

But then dispose will be synchronous! Because public method DisposeAsync() will call sync version of dispose on line 538.

[ShreyasJejurkar]

@pranavkm I have removed DisposeAsyncCore() method in the latest commit. But right now DisposeAsync() method calling the synchronous version of Dispose() under the hood, isn't that wrong? Or am I missing something here to understand!
Thanks!

[javiercn]

Coming back to this, I think the pattern should be something like this, based on:

• implement-both-dispose-and-async-dispose-patterns
• Utf8JsonWriter

The main points are:

• Dispose(bool) always disposes things that implement IDisposable when disposing = true and just releases unmanaged resources otherwise.
• Dispose(bool) disposes async resources and blocks if Dispose() was invoked instead of DisposeAsync()
  • This can likely be pushed to Dispose() however derived classes should call base.Dispose(disposing)
• DisposeAsync invokes DisposeAsyncCore to dispose IAsyncDisposable values, sets a flag for Dispose(disposing) to avoid invoking DisposeAsyncCore and blocking, calls Dispose(disposing: true) and then `GC.SupressFinalize(this).
• The finalizer only releases unmanaged resources for which there are no asynchronous versions.

@davidfowl thoughts?

public class MyClass : IDisposable, IAsyncDisposable
{
    private bool _disposed = false;
    private bool _disposeAsync = false;

    public void Dispose()
    {
        if(_disposed)
        {
            return;
        }

        Dispose(disposing: true);
        GC.SupressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if(disposing)
        {
            if(!_disposeAsync)
            {
                DisposeAsyncCore()
                    .ConfigureAwait(false)
                    .GetAwaiter()
                    .GetResult();
            }
        }

        // Release unmanaged resources
    }

    public virtual ValueTask DisposeAsyncCore() => return default;

    public ValueTask DisposeAsync()
    {
        if(_disposed)
        {
            return;
        }
        if(_disposeAsync)
        {
            return;
        }
        _disposed = true;

        await DisposeAsyncCore().ConfigureAwait(false);
        _disposeAsync = true;
        
        Dispose(disposing: true);
        GC.SupressFinalize(this);
    }

    ~MyClass
    {
        Dispose(disposing: false);
    }
}

[davidfowl]

Remove this, there's no finalizer.

[davidfowl]

We want to actually call DisposeAsync on the host.

[davidfowl]

Make sure this adds a service that is only IAsyncDisposable, that's the real test.

[ghost]

Hi @MCCshreyas.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.