IAsyncFtpClient defines duplicates for Dispose and DisposeAsync

[kowalski-se]

FTP Server OS: Not relevant

FTP Server Type: Not relevant

Client Computer OS: Not relevant

FluentFTP Version: 50.0.0; 50.0.1; 50.1.0; 51.0.0; 51.1.0

Framework: .NET Standard 2.0, but applicable to any of the newer frameworks

The interface IAsyncFtpClient redefines the Dispose() method and also defines an improper DisposeAsync() method:

First, to the Dispose() method: Defining the method again with a "new" keyword leads to two declarations of the method, i.e. IAsyncFtpClient.Dispose() and IDisposable.Dispose(). While I'm not sure if this is connected properly without introducing bugs, it definitely introduces issues when unit testing. Before version 50.0.0, we could use following validation:

var ftpClientMock = A.Fake<IAsyncFtpClient>();
// Some testing calls
A.CallTo(() => ftpClientMock.Dispose()).MustHaveHappenedOnceExactly();

This is not longer possible as the implementation using var ftpClient = new AsyncFtpClient(...); will only call IDisposable.Dispose() and not IAsyncFtpClient.Dispose(), since we only fake the interfaces and do not have a definition like void IDisposable.Dispose() => Dispose(); which will forward the dispose call to IAsyncFtpClient.Dispose(). So now we need to verify the explicit call:

A.CallTo(() => ((IDisposable)ftpClientMock).Dispose()).MustHaveHappenedOnceExactly();

As mentioned, I'm not sure if this also introduces further disposal bugs since most IoC containers dispose their instances in some logic like this:

foreach (var obj in container.Instances)
{
    if (obj is IDisposable disposable)
    {
        disposable.Dispose();
    }
}

This will also only call the IDisposable.Dispose() method as long as this does not implement a forward to IAsyncDisposable.Dispose()

My suggestion here is to simply remove the redefinitions, which should neither introduce build errors nor any further code changes:

Second, to the improper DisposeAsync() method: This is really .NET Standard 2.0 (and below) specific. The method Task DisposeAsync() defined in IAsyncFtpClient somehow allows to use await using var ftpClient = new AsyncFtpClient(...), but when executing this code, it throws a MissingMethodException because the interface IAsyncDisposable does not exist before .NET Standard 2.1:

So I'd suggest to remove the DisposeAsync method for .NET Standard 2.0 and below entirely from the interface. It can remain in AsyncFtpClient, but perhaps it makes more sense to define it as non-public.

If you need more infos, don't hesitate to ask :)

Logs : No logs are required here as this is a build-time issue.

[FanDjango]

Well, thanks for the feedback. I was waiting with some apprehension to finally hear from those out there as to how the changes since V50 were working out. As you might have seen, they were provoked by some users reporting that async disposal was really not up to par in the previous releases.

I more or less understand the things you have described and have decided to implement both your suggestions.

Therefore, the definition of ValueTask DisposeAsync() and Task DisposeAsync() in the interface have simply been completely removed and some of the client dispose logic changed to reflect the .NET Standard 2.0 and below consideration, the change is merged. Let's see where that takes us, maybe you can pick it up from GitHub and continue this from there.

[kowalski-se]

That was quick, thanks a lot :) Can you also publish a new NuGet package including the changes? :)

[FanDjango]

I am afraid that no NuGet releases are planned before mid-November.

Have you verified that the changes did work for you? I suppose so, as you have closed this issue.

[kowalski-se]

Alright no hurry, thanks for the update :) Yes, all verified and working as expected :)