cwmiller/broadworks-connector-net

Cancelling a Request Sent Over SoapTransport Can Crash Application

Opened this issue · 1 comments

I'm seeing calls to SoapTransport's SendAsync method crash when the cancellation token passed into the method is cancelled. I presume this is because the cancellationToken.ThrowIfCancellationRequested(); is being called inside the callback action passed to the cancellation token's Register method, which may cause an unhandled exception that cannot be propagated back onto the SendAsync method.

Here's a suggestion from my currently workaround, in case it is helpful: Change SoapTransport's SendAsync to cancel asyncronously without throwing in the callback (this is written for netstandard2.0, there is some simplifications that can be done in later .Net frameworks):

public async Task<string> SendAsync(string request, CancellationToken cancellationToken = default)
{
    cancellationToken.ThrowIfCancellationRequested();

    // Register the cancellation token callback to cancel the completion source that will also be awaited.
    var cancelationCompletionSource = new TaskCompletionSource<processOCIMessageResponse>();
    using (var ctr = cancellationToken.Register(() => cancelationCompletionSource.TrySetCanceled()))
    {
        var task = _client.processOCIMessageAsync(request);

        // The completed task will be either the task for the request or the cancellation completion source's cancelled task
        // that will cause an OperationCanceledException to be thrown when it is awaited.
        var completedTask = await Task.WhenAny(task, cancelationCompletionSource.Task).ConfigureAwait(false);
        processOCIMessageResponse response = await completedTask.ConfigureAwait(false);
        return response?.Body?.processOCIMessageReturn ?? throw new BadResponseException("No processOCIMessageReturn in response");
    }
}

Now I'd like to point out this doesn't call _client.Abort(), so the request will continue on. Ideally the request should be aborted but if there are other requests in flight through the transport that should not be aborted, does not aborting this protect those? It may be helpful to make the BWProvisioningServiceClient disposable, and have its Dispose method call Abort() on itself, and have the SoapTransport call _client.Dispose() in its own Dispose method in order to terminate all requests.

Thanks for reporting this.

I'm not exactly sure what the behavior of _client.Abort() is, but yeah it does seem like it would cancel all running requests. To be honest, I don't really use the SOAP transport so I hadn't ran into these quirks before.

I'm looking at rewriting SoapTransport to utilize HttpClient rather than the Connected Services system. That way there's much more control over the request, including being able to cancel specific ones.