A couple of changes/fixes in SshCommand #1423
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is it ready for review? |
|
Yes, thanks |
WojciechNagorski
approved these changes
Jun 9, 2024
|
Thanks |
Rob-Hague
added a commit
to Rob-Hague/SSH.NET
that referenced
this pull request
Jun 14, 2024
After the previous change (sshnet#1423), this basically entails swapping out the IAsyncResult for a TaskCompletionSource and hooking up the cancellation/timeout logic. As with the prior Begin/End pattern, the initiation of the command (SendExecRequest) happens synchronously, so there's a bit of room for improvement there, but otherwise it is the Task-based async that we know and like. I chose to make it void (Task)- returning instead of string like in the existing overloads, so that OutputStream is not automatically consumed (and encoded as a string) when that may not be desired. As in sshnet#650, I was initially considering changing the other overloads to be void-returning as well, but decided that it was not worth the break since most people will probably want to change over to ExecuteAsync anyway.
Rob-Hague
added a commit
to Rob-Hague/SSH.NET
that referenced
this pull request
Jun 14, 2024
After the previous change (sshnet#1423), this basically entails swapping out the IAsyncResult for a TaskCompletionSource and hooking up the cancellation/timeout logic. As with the prior Begin/End implementation, the initiation of the command (SendExecRequest) happens synchronously, so there's a bit of room for improvement there, but otherwise it is the Task-based async that we know and like. I chose to make it void (Task)- returning instead of string like in the existing overloads, so that OutputStream is not automatically consumed (and encoded as a string) when that may not be desired. As in sshnet#650, I was initially considering changing the other overloads to be void-returning as well, but decided that it was not worth the break since most people will probably want to change over to ExecuteAsync anyway.
This was referenced Jun 14, 2024
WojciechNagorski
added a commit
that referenced
this pull request
Jun 19, 2024
* Add SshCommand.ExecuteAsync After the previous change (#1423), this basically entails swapping out the IAsyncResult for a TaskCompletionSource and hooking up the cancellation/timeout logic. As with the prior Begin/End implementation, the initiation of the command (SendExecRequest) happens synchronously, so there's a bit of room for improvement there, but otherwise it is the Task-based async that we know and like. I chose to make it void (Task)- returning instead of string like in the existing overloads, so that OutputStream is not automatically consumed (and encoded as a string) when that may not be desired. As in #650, I was initially considering changing the other overloads to be void-returning as well, but decided that it was not worth the break since most people will probably want to change over to ExecuteAsync anyway. * Update examples --------- Co-authored-by: Wojciech Nagórski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CancelAsync, we were sendingexit-signalchannel request, when in fact we should be sendingsignaland the server will respond withexit-signal. This explains why we did not see any response or channel close in Fix CancelAsync Cause Deadlock #1345 (@zeotuan fyi). Add a correspondingstring? ExitSignalproperty which has a value when the server sends one. (closes ExitSignalName #95 - seems like it existed once upon a time).IAsyncResult.WaitHandleonly when the command had completed successfully, or otherwise onceEndExecutehad been called. But we should be setting it as soon as the command completes whether successfully, with an error or via cancellation. This refactors the logic by removing the extra wait handles and consolidating the completion logic onto theIAsyncResult.WaitHandle. With this refactoring the logic intentionally looks (with some squinting) similar to aTask. AddingExecuteAsyncshould only be a small step from here.ExitStatusby changing the return type frominttoint?, in order to distinguish between an exit status of 0 and when no exit status has been returned (because for example, an exit signal was returned instead). This is the second commit.