Add ExecuteAsync, Fix CancelAsync Deadlock #1343
Conversation
zeotuan
commented
Mar 5, 2024
zeotuan
commented
- Implemented TAP over APM ExecuteAsync(CancellationToken) mentiond in Add async support for Connect() function? #931
- Fix CancelAsync causing deadlocking and it now send SIGTERM signal to try to abort the request which will probably also solve CancelAsync does not cancel the command started by BeginExecute and blocks execution #1147 and Is it possible to cancel SshCommand? #1023
zeotuan
force-pushed
the
develop
branch
2 times, most recently
from
751d5ad
to
c04d6fb
Compare
* Add ExecuteAsync with cancellation * Deprecate Error and Result property * Fix Deadlock with cancel Async
|
|
||
| try | ||
| { | ||
| var status = await Task<int>.Factory.FromAsync(BeginExecute(), EndExecuteWithStatus).ConfigureAwait(false); |
There was a problem hiding this comment.
I want to add Async overloaded methods but I think this is not a real async code. I've prepared a POC here: #1318
I know that your approach is easier to implement, but this is similar to nuget: https://www.nuget.org/packages/Renci.SshNet.Async#versions-body-tab
You can find code here: https://github.com/JohnTheGr8/Renci.SshNet.Async/blob/master/Renci.SshNet.Async/SshNetExtensions.cs
| @@ -105,56 +107,72 @@ public Stream CreateInputStream() | |||
| /// <summary> | |||
| /// Gets the command execution result. | |||
| /// </summary> | |||
| #pragma warning disable S1133 // Deprecated code should be removed | |||
| [Obsolete("Please read the result from the OutputStream. I.e. new StreamReader(shell.OutputStream).ReadToEnd().")] | |||
There was a problem hiding this comment.
Why should we make it obsolete?
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the command execution error. | ||
| /// </summary> | ||
| #pragma warning disable S1133 // Deprecated code should be removed | ||
| [Obsolete("Please read the error result from the ExtendedOutputStream. I.e. new StreamReader(shell.ExtendedOutputStream).ReadToEnd().")] |
There was a problem hiding this comment.
Why should we make it obsolete?
|
@WojciechNagorski commented just as I was commenting some similar feedback, so sorry for the duplication 🙂 : I think you are still working on this so just some high-level thoughts: There are a few semi-related changes here. The cancellation via signal is good. I would suggest splitting that to a standalone PR to start with. I understand the reason for obsoleting the Result/Error properties (because they consume the stream properties) but I do not think the change is justified. Especially given how many changes it causes just in our own code (forwarding to a now internal version at that). I would suggest reverting that. I'm a little uneasy about using |
|
@Rob-Hague @WojciechNagorski Very Interesting. I would love to help with the project. https://www.nuget.org/packages/Renci.SshNet.Async#versions-body-tab unfortunately does not support I also expect the execution of command to leave the Therefore, I would suggest Future ExecuteAsync Implementation to at least just return an exit status code instead of consuming those stream. Also Providing a public helper method to let people get Result/Error from those stream. It would require refactoring for any downstream project that will migrate to Async anyway I have submitted a standalone PR for CancelAsync: #1345 |
|
@zeotuan Sory for the delay. If you want you can continue to help us add true asynchronous methods. let us know. |
|
@WojciechNagorski Hi! Yes, I would like to help with adding true async methods for sshnet. |
