ObjectDisposedException is thrown instead of TaskCancelledException when FtpClient.ConnectAsync() is called with cancelled token

[RedwoodForest]

FTP OS: N/A

FTP Server: N/A

Computer OS: Windows

FluentFTP Version: 32.3.3

When FtpClient.ConnectAsync() is called with a CancellationToken that has already been cancelled it throws ObjectDisposedException. I was expecting TaskCancelledException.

I discovered this during development while testing the FtpClient's cancellation behavior, but it seems plausible that it could happen in a production app, e.g., if multiple operations are using the same cancellation token and the token happens to be cancelled just before the call to ConnectAsync().

Here's code that reproduces the issue:

private static async Task TestAsync() {

    FtpTrace.AddListener(new ConsoleTraceListener());

    // This could be cancelled due to multiple operations using the same token, a timeout expiring, etc.
    var cancelledToken = new CancellationToken(true);

    using (var ftpClient = new FtpClient(new Uri("ftp://127.0.0.1"))) {
        await ftpClient.ConnectAsync(cancelledToken);
    }
}

Logs :

# ConnectAsync()
Status:   Connecting to 127.0.0.1:21

# Dispose()
Status:   Disposing FtpClient object...
Status:   Disposing FtpSocketStream...
Status:   Disposing FtpSocketStream...

Here's the exception:

System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'System.Net.Sockets.Socket'.
   at System.Net.Sockets.Socket.InternalEndConnect(IAsyncResult asyncResult)
   at System.Net.Sockets.Socket.EndConnect(IAsyncResult asyncResult)
   at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FluentFTP.FtpSocketStream.<EnableCancellation>d__64.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FluentFTP.FtpSocketStream.<ConnectAsync>d__80.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FluentFTP.FtpClient.<ConnectAsync>d__29.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at FluentFTP.FtpClient.<ConnectAsync>d__27.MoveNext()
[...non FtpClient stack traces omitted...]

[robinrodricks]

Hi, thanks for this. So can this fix it?

At the start of every async method, this snippet:

if (CancellationToken.IsCancellationRequested){
    throw new TaskCancelledException();
}

[RedwoodForest]

Hi, thanks for taking a look at this.

That would fix the specific case where the token passed in is already cancelled (though I would use CancellationToken.ThrowIfCancellationRequested() instead of the code from your comment), but it doesn't fix the case where the token is cancelled from another thread while ConnectAsync is running.

Based on the documentation it sounds like the recommended pattern is to handle the ObjectDisposedException used to indicate cancellation:

To cancel a pending call to the BeginConnect method, close the Socket. When the Close method is called while an asynchronous operation is in progress, the callback provided to the BeginConnect method is called. A subsequent call to the EndConnect method will throw an ObjectDisposedException to indicate that the operation has been cancelled.

What about something like this?

try {
    await EnableCancellation(Task.Factory.FromAsync(connectResult, m_socket.EndConnect), token, () => m_socket.Close());
} catch (ObjectDisposedException) {
    token.ThrowIfCancellationRequested();
    // If this code is reached it means that the call to m_socket.Close() was not from
    // EnableCancellation() (because the token is not cancelled).
    // I haven't checked the rest of the code to determine if this is possible or if this is 
    // the correct way to handle it, but it will match the behavior from before this change.
    throw;
} 

[robinrodricks]

This might solve it. You want me to use this pattern for every method? Or just a change in the internal socket methods?

[RedwoodForest]

It seems to me like it should be used anywhere ObjectDisposedException would be thrown instead of TaskCancelledException when an in-progress operation is cancelled.

It seems like would apply anywhere Socket.Close() is used to abort an in-progress operation when it is cancelled (e.g. via EnableCancellation). I'm not familiar enough with the code to know if there are other places where this pattern is used. I also have not looked into how this applies to the .NET Core version of the code.