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

Make ServiceChannelProxy implement IAsyncDisposable #5385

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Dec 14, 2023

So that channels created through a ChannelFactory<T> without inheriting from ClientBase<T> can also benefit from the asynchronous disposal improvements introduced in #4865.

Fixes #5270

So that channels created through a `ChannelFactory<T>` without inheriting from `ClientBase<T>` can also benefit from the asynchronous disposal improvements introduced in dotnet#4865.

Fixes dotnet#5270
0xced added a commit to 0xced/Wcf.HttpClientFactory that referenced this pull request Dec 15, 2023
Pros:
* Consumer doesn't need to set Client.CacheSetting = CacheSetting.AlwaysOn
* Consumer doesn't need to add a constructor that takes a ServiceEndpoint to their partial client class

Cons:
* Throws on disposal if the channel is in a faulted state (until dotnet/wcf#5385 is merged)
@0xced
Copy link
Contributor Author

0xced commented Feb 6, 2024

Is there any chance for this to eventually land in a 8.x.x update of the System.ServiceModel NuGet packages?

@0xced
Copy link
Contributor Author

0xced commented Feb 21, 2024

Here's why this pull request is important. 😁 https://github.com/0xced/Wcf.HttpClientFactory/blob/6254a6df13fddcd3143c0ece726ccd5778ff15c9/tests/B2BServiceTest.cs#L86-L90

There's a lot going on in my Wcf.HttpClientFactory so please don't hesitate to ask if you have questions.

@mconnew
Copy link
Member

mconnew commented Mar 21, 2024

This will go into our next 8.x.x release. We don't have a planned release right now, so this will go out in the next release that happens.

@mconnew mconnew merged commit 0fdce35 into dotnet:main Mar 21, 2024
8 checks passed
@mconnew
Copy link
Member

mconnew commented Mar 21, 2024

Btw, I think you can work around this by creating another type in DI which wraps the channel and implements IAsyncDisposable. In that, it does a non-throwing Dispose of the client. If I remember right, objects get disposed in a predictable order, I think it's construction order. But if it's the other way around, you can accommodate that too. You would something like this:

internal class WcfThrowingDisposeWrapper : IAsyncDisposable
{
    public CommunicationObject Disposable { get; set; }
    public ValueTask DisposeAsync()
    {
        if (Disposable == null) return;
        try
        {
            // Only want to close if it is in the Opened state
            if (Disposable.State == CommunicationState.Opened)
            {
                await Task.Factory.FromAsync(Disposable.BeginOpen, Disposable.EndOpen, null);
            }
            // Anything not closed by this point should be aborted
            if (Disposable.State != CommunicationState.Closed)
            {
                Disposable.Abort();
            }
        }
        catch (CommunicationException)
        {
            Disposable.Abort();
        }
        catch (TimeoutException)
        {
            Disposable.Abort();
        }
    }
}

In the place where you add a factory to create the WCF client, you also create an instance of WcfThrowingDisposeWrapper and set the WCF client on it. Something like this:

services.AddScoped<WcfThrowingDisposeWrapper>();
services.AddScoped<IService>(services =>
{
    var wrapper = services.GetRequiresService<WcfThrowingDisposeWrapper>();
    var channel = services.GetRequiresService<ChannelFactory<IService>>();
    wrapper.Disposable = channel;
    return channel;
});

When you dispose the scope, it should dispose the WcfThrowingDisposeWrapper first which should dispose the channel without throwing, then when the scope comes to dispose the channel, it will already have been done. The in-box DI service provider scope maintains a simple List<object> for all instantiated instances which are disposable, adding them as they are created. Then when the scope is disposed, it iterates the list and disposes them, which means WcfThrowingDisposeWrapper would get disposed first. You should still keep your catch block in your implementation as the order of disposal is an internal implementation detail and a 3rd party DI engine may not do it in the same order.

@0xced 0xced deleted the ServiceChannelProxy+IAsyncDisposable branch March 26, 2024 08:51
@0xced
Copy link
Contributor Author

0xced commented Nov 8, 2024

.NET 9 is going to be released very soon. Are there plans to release either a 8.x.x update with this fix included, or maybe a 9.0.0 version?

0xced added a commit to 0xced/Wcf.HttpClientFactory that referenced this pull request Nov 8, 2024
Matt Connew, maintainer of WCF, said that this should be fixed in the next System.ServiceModel.Primitives version.

> This will go into our next 8.x.x release. We don't have a planned release right now, so this will go out in the next release that happens.

See dotnet/wcf#5385 (comment)
@0xced
Copy link
Contributor Author

0xced commented Nov 13, 2024

I see that System.ServiceModel.Primitives 8.1.0 has been released and includes this fix. 🥳

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

Successfully merging this pull request may close these issues.

ServiceChannelProxy should implement both IAsyncCommunicationObject and IAsyncDisposable
2 participants