sshnet / SSH.NET

SSH.NET is a Secure Shell (SSH) library for .NET, optimized for parallelism.
http://sshnet.github.io/SSH.NET/
MIT License
3.96k stars 931 forks source link

Add ExecuteAsync, Fix CancelAsync Deadlock #1343

Closed zeotuan closed 3 months ago

zeotuan commented 6 months ago
Rob-Hague commented 6 months ago

@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.

zeotuan commented 6 months ago

@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

WojciechNagorski commented 6 months ago

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

zeotuan commented 6 months ago

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