SSH.NET icon indicating copy to clipboard operation
SSH.NET copied to clipboard

Add ExecuteAsync, Fix CancelAsync Deadlock

Open zeotuan opened this issue 1 year ago • 4 comments

  • Implemented TAP over APM ExecuteAsync(CancellationToken) mentiond in https://github.com/sshnet/SSH.NET/issues/931
  • Fix CancelAsync causing deadlocking and it now send SIGTERM signal to try to abort the request which will probably also solve https://github.com/sshnet/SSH.NET/issues/1147 and https://github.com/sshnet/SSH.NET/issues/1023

zeotuan avatar Mar 05 '24 02:03 zeotuan

@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 TaskFactory.FromAsync in this library because many of the Begin/End (APM) methods are not implemented so well and we end up spawning new threads/blocking on more threads than we need. I basically share the same sentiment as https://github.com/sshnet/SSH.NET/issues/931#issuecomment-1066160452. But at first glance it is not so bad here.

Rob-Hague avatar Mar 06 '24 14:03 Rob-Hague

@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 CancellationToken. So I was thinking of adding a simple TAP over APM wrapper here until we get a proper TAP rewrite.

I also expect the execution of command to leave the OutputStream and ExtendedOutputStream intact and provide my own way to consuming them. for example: I want to forward the stdout and stderr to another program while the command is being executed not after.

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: https://github.com/sshnet/SSH.NET/pull/1345

zeotuan avatar Mar 06 '24 23:03 zeotuan

@zeotuan Sory for the delay. If you want you can continue to help us add true asynchronous methods. let us know.

WojciechNagorski avatar Mar 18 '24 08:03 WojciechNagorski

@WojciechNagorski Hi! Yes, I would like to help with adding true async methods for sshnet.

zeotuan avatar Mar 22 '24 07:03 zeotuan