Feature/executeasync

[kendallb]

No description provided.

[IgorMilavec]

I don't think we should have an overload that provides the commandText. commandText is already provided in constructor and allowing it to be changed here again adds to confusion.

[kendallb]

I can certainly remove that, but I was just mirroring the regular Execute() API functions and there is an overload for that already?

SSH.NET/src/Renci.SshNet/SshCommand.cs

Line 357 in bc99ada

public string Execute(string commandText)

[IgorMilavec]

Too much flexibility here, can we just provide a simple '''Task ExecuteAsync(CancellationToken cancellationToken)```? I would also specifically not provide a default for cancellationToken, making the caller aware that this task might never complete and they should provide some means of timeout.

[kendallb]

Sure, those were options in the TAP wrapping code I found, but can certainly get removed.

[IgorMilavec]

I really don't like using these extensions... we can use builtin Task<int>.Factory.FromAsync(). The only "advantage" these extensions have is that on the surface they seem to support cancellation, but this is not actually true, the channel will not be closed in this case. I'll try to provide an example that closes the channel.

[IgorMilavec]

This is how I propose to implement it:

public async Task<int> ExecuteAsync(CancellationToken cancellationToken)
{
    var ctr = cancellationToken.Register((c) => ((IChannelSession)c).Dispose(), _channel, false);
    try
    {
        return await Task<int>.Factory.FromAsync(BeginExecute(), EndExecuteWithStatus).ConfigureAwait(false);
    }
    catch (Exception) when (cancellationToken.IsCancellationRequested) // ToDo: Define a more specific exception
    {
        throw new OperationCanceledException("Command execution has been cancelled.", cancellationToken);
    }
    finally
    {
        ctr.Dispose();
    }
}

No extensions required and it should handle cancellation correctly, Please note that this has not been tested.

[kendallb]

I think the point of the extension method is that TaskFactory.FromAsync() by default does not support cancellation tokens at all, so if you use one say for a timeout, it won't do anything. The code I am using does support that. I tried to implement closing of the session by setting the wait handle, but we could register the channel to be closed when it gets canceled also. I can play around with this some more. But your code as it stands I don't think will do anything useful because the cancellation token is not actually used anywhere?

I know I tested timeout with the code as is and it works (I wrote a unit test for it), but it's possible the channel is not getting closed.

[kendallb]

Ok yeah, the above code did not work. I have re-written it using a modified version of the extension which works correctly. To make it work you still need to set up a second task for the cancelation operation so you can determine if the task got cancelled, but this new version uses a callback to cancel the operation (closes the channel) and it works. Since we are actually closing the channel and terminating the original operation. I have verified the channel is being properly closed with this iteration.

I did leave the code as an extension as it would be useful to do use the same for other operations? But if you want I can simply remove the extension and implement the same code in the function itself? Either way the approach taken to set up a separate cancellation task so you know if the cancellation is needed is what works. Otherwise the original task just blocks until it's fully complete and the cancellation token does nothing.

[IgorMilavec]

I think you are missing the point about cancellation: cancellation in dotnet is cooperative. If the underlying code doesn't observe it, the task will not get canceled. This is what I was proposing to cancel the channel in case of cancellation. There might be better options in SSH protocol, but this could do for now.

[kendallb]

Right, but cancelling the channel itself did not seem to early out of the await, which I think is the primary reason for the code I found and used. It creates a separate task that will bail when the cancellation token fires, and you can then dispose of the channel and return from the await. When I tried just closing the channel when the cancelation token fired, the await did not return until the /bin/sleep 5 completed. The way it is written now it works. The awaited task returns as soon as the cancelation token fires, and the channel also gets properly closed.

[IgorMilavec]

Yes, but it is very "cludgy". If disposing the channel doesn't terminate the wait in EndExecute we have a problem that we need to solve first. I wouldn't want to layer patch-fixes on top to hide this problem.

[kendallb]

Fair enough. I don't know enough about async programming to debug why that is not happening unfortunately.

[kendallb]

Ok I debugged it again. Part of the problem when I was debugging it last time was that the registration on the cancelation token was done before the command was started, so since I have such a short timeout in my test (100ms) by the time I debug that code it runs immediately, but there is no channel to close. So I changed it slightly to this:

            var asyncResult = BeginExecute();
            var ctr = cancellationToken.Register(_ => _channel?.Dispose(), false);
            try
            {
                return await Task<int>.Factory.FromAsync(asyncResult, EndExecuteWithStatus).ConfigureAwait(false);
            }
            catch (Exception) when (cancellationToken.IsCancellationRequested) // ToDo: Define a more specific exception
            {
                throw new OperationCanceledException("Command execution has been cancelled.", cancellationToken);
            }
            finally
            {
                ctr.Dispose();
            }

Now when it fires it has a channel to dispose and I can see that it is terminating the command and the await does actually finish and return as soon as the cancellation occurs, so that part was incorrect on my part. But the real problem is there is no cancellation exception happening so the await just returns and we never get into the catch block. In the test, it comes out and I get a blank string for the result, so it fails the test as I never got a cancellation exception.

With the way I have it structured in this PR, the channel is dispose and closed, the original await does finish but because we also run waited on another task to capture just the cancellation we now get the correct exception so the caller knows the command was cancelled.

I am not sure how else you would get the original await to throw the correct exception without doing that, since TaskFactory.FromAsync does not natively support a cancellation token.

[kendallb]

The other way this could potentially work is if there was a way to flag the running task that it was cancelled, and then you wake it up by setting the wait handle it's waiting on. It sees that it has been cancelled and then throws the appropriate exception that we want to indicate it was cancelled?

[kendallb]

The unit test I have in there for the cancellation assumes a Unix on the other end so it can call /bin/sleep to cause a long running command so we can test cancellation with a shorter timeout. I am not sure how to make that work otherwise, but it does test the functionality. So it could be left or perhaps set as ignored or something depending on how you normally run the tests?

[IgorMilavec]

Please clean up the PR:

• Remove .idea/* files
• Remove AsyncExtensions.cs as it is not needed any more

[IgorMilavec]

This is the sync version, why are you changing the title of examples to "async"?

[kendallb]

Flattened all the code and force pushed it up now so it's clean.

[IgorMilavec]

Please provide a cancellationToken, something like
CancellationTokenSource cancellationTokenSource = new CancellationTokenSource(TimeSpan.FromSeconds(60));

[kendallb]

Ok done.

[IgorMilavec]

I propose to not provide a default cancellationToken. I real-life networking nothing really works all the time, and users are not aware that by not providing the CancellationToken, they are willing to wait forever. And waiting forever is almost never what they really wanted. And if somebody really wants to wait forever, he/she can still explicitly specify the default...

[kendallb]

Agreed. I already removed it on sshCommand.Execute but forgot for this one.

[kendallb]

All good points. I will get it adjusted this weekend.

[kendallb]

Ok cleaned up and force pushed. The one issue is that now that Result is obsolete, I had to disable the complaint about using it in EndExecute as that is still written to return the result string. It gets it compiling but not sure it's the best approach?

[kendallb]

Not sure how to best handle this, but since this is obsolete, we have to stop the compiler from giving a warning here as the project will not compile otherwise.

[IgorMilavec]

We could have private GetResult() that would be called from here and from Result.

[IgorMilavec]

When CancellationTokenSource is used with timeout, it should be disposed, otherwise Timers will pile up. SOmething like:

using (var cts = new CancellationTokenSource(timeout))
{
    await SomethingAsync(cts.Token);
}

[IgorMilavec]

Is there something to discuss here? Console.WriteLine and Inconclusive...

[IgorMilavec]

The more I think about it I don't feel we should have such simple helpers... yes, this is aligned with the sync version, but... something to discuss with Gert, I think.

[IgorMilavec]

We could have private GetResult() that would be called from here and from Result.