Issue when ConnectAsync() gets already cancelled CancellationToken

[svakrv]

Hey,

If I call ConnectAsync with already cancelled CancellationToken, I get stuck and keep waiting for the returned task to end for infinity time.

var cancellationToken = new CancellationToken(true); // cancelled = true
connection = await dialer.ConnectAsync(cancellationToken);

I think this is a problem that class DefaultRasDialCallbackHandler do not catch exceptions for all parts of code. Method WaitForHandleToBeTransferred() throws the OperationCanceledException but since this part of code has no handling of exceptions, it will never end the completionSource and the client will stay waiting for ConnectAsync to finish forever.

Maybe it could be fixed something like this:

public bool OnCallback(IntPtr dwCallbackId,, int dwSubEntry, IntPtr hrasconn, uint message, RasConnectionState rascs, int dwError, int dwExtendedError)
{
    try
    {
        GuardMustNotBeDisposed();
        GuardMustBeInitialized();
        WaitForHandleToBeTransferred();

        GuardRequestShouldNotBeCancelled();
        GuardErrorCodeMustBeZero(dwError);

        ExecuteStateChangedCallback(rascs);

        if (HasConnectionCompleted(rascs))
        {
            SetConnectionResult();
        }
    }
    catch (Exception ex)
    {
        HangUpConnection();
        SetExceptionResult(ex);
    }            
    
    return !Completed;
}

[jeff-winn]

Thanks for the bug report, I’ll see if I can get the same results

[jeff-winn]

Yeah, it's definitely a confirmed bug. I hadn't expected the cancellation token to already be cancelled when the request comes in.

However it does pose an interesting (and rather difficult) problem on ensuring the resources (like the EAP security token, connection, and other handles) get cleaned up if the API calls from Windows hang (which means DotRas never has that callback method execute). There's some code up in the RasDialService to help deal with that, but with it decentralized it's a little hard to manage.

A couple things I probably need to do:

• Break the code apart to centralize the resource cleanup (perhaps a RasDialCancellationService).
• Check the cancellation token and throw if cancelled prior to the linked cancellation source on the RasDialService being setup and the callback registered.
• The guard checks outside of the try block should likely be redesigned such that the callback can simply return false so Microsoft ceases calling the function. The disposal logic already in place on the dialer should take care of the resource cleanup.
• As you mentioned, the WaitForHandleToBeTransferred method may need to move into the try block, but that's only due to the callback handler having part of the hangup logic. The first item mentioned may negate the need for this being there depending on the architectural changes made.

Might have to think about this for a few days, depending on the change it could cause a race condition with the same resulting underlying behavior.

[Cekdisout]

Hi,
what if you add additional catch for OperationCanceledException?
It will cancel connection.

public bool OnCallback(
        IntPtr dwCallbackId,
        int dwSubEntry,
        IntPtr hrasconn,
        uint message,
        RasConnectionState rascs,
        int dwError,
        int dwExtendedError)
    {
        GuardMustNotBeDisposed();
        GuardMustBeInitialized();

        WaitForHandleToBeTransferred();

        try
        {
            GuardRequestShouldNotBeCancelled();
            GuardErrorCodeMustBeZero(dwError);

            ExecuteStateChangedCallback(rascs);

            if (HasConnectionCompleted(rascs))
            {
                SetConnectionResult();
            }
        }
        catch (OperationCanceledException operationCanceledEx)
        {
            HangUpConnection();
            SetExceptionResult(operationCanceledEx);
        }
        catch (Exception ex)
        {
            HangUpConnection();
            SetExceptionResult(ex);
        }
        finally
        {
            if (Completed)
            {
                RunPostCompleted();
            }
        }

        return !Completed;
    }