Async PoC #1318
Async PoC #1318
Conversation
|
Performance and allocation look similar:
|
|
In my personal opinion, I believe that trying to introduce Task within the existing code base will be quite a challenge as you'll face a lot of collision. The repo is almost 10 years old now, so there's bound to be a lot of things that if they had been written today, they'd have been done differently. I also believe that in its essence, it is in fact worth the effort to bring to life a Task based version of this library. The entire .NET eco system is now aligned around Task (except for some very high performance low level stuff). However, it may be more time efficient to simply write it from scratch and use the old code base as a reference. |
|
@jscarle I admit that I was already thinking whether it would be easier to create a new repository. Nowadays, there are now channels, better synchronization tools, better socket support. In Poland, we now have a competition to create 100 commits in 100 days. https://100commitow.pl, that's why I started thinking about it. But it's a bit crazy to take on such a large project yourself. :) In fact, it's no less crazy to add full async support to ssh.net. ;) |
|
I think it's perfectly possible to get proper async going in SSH.NET. WaitHandles can easily be wrapped in Tasks if needed (https://github.com/StephenCleary/AsyncEx/blob/master/src/Nito.AsyncEx.Interop.WaitHandles/Interop/WaitHandleAsyncFactory.cs). I think it's a worthwhile mission. I also don't see anything that requires NET6_0_OR_GREATER, but I haven't looked at it closely. I will try to take a proper look in a few days. |
| throw new SshConnectionException("Client not connected."); | ||
| } | ||
|
|
||
| await SocketAbstraction.SendAsync(_socket, data, token).ConfigureAwait(false); |
There was a problem hiding this comment.
This method is implemented only on .NET 6+ That's why I've implemented the entire mechanism for .NET 6+.
There was a problem hiding this comment.
We can fix that. I have a branch (develop...Rob-Hague:SSH.NET:sockets) which reimplements SocketExtensions for lower targets using tasks. It has ConnectAsync and ReceiveAsync - we can add SendAsync.
There was a problem hiding this comment.
I would like to add methods from https://www.nuget.org/packages/Renci.SshNet.Async
But I would like to do it better than in this extensions:
There was a problem hiding this comment.
I'll clean up my branch over the next few days and add a SendAsync
|
I think there'd be a lot of benefit in starting a new repository and separating things into various smaller projects, sort of the way that Microsoft divides things into a lot of different parts (ie: EntityFramework):
We could separate out the base protocol classes from the encryption, SFTP, SCP, and so on. |
|
We could also use it as an opportunity to support only .NET 6+ (or even .NET 8+) thus splitting this project into a legacy code base and a modern one. |
There was a problem hiding this comment.
I think it looks good as a proof of concept. On a high level:
- I think we should try to make it work for all frameworks and overcome any obstacles as we hit them.
- I think a reasonable plan of attack would be to go from the "inside-out", i.e. add async to the internals and work outwards to the public API
Both Session.MessageListener and Session.ReceiveMessage could be made async without any need for a duplicate sync version.
Then we can look at making Session.WaitOnHandle to be Task-based.
Then we can look at Channel, and then the public stuff on SshClient, SftpClient etc. should come naturally
| #if NET6_0_OR_GREATER | ||
| _ = Interlocked.Increment(ref _outboundPacketSequence); | ||
| #else | ||
| _outboundPacketSequence++; | ||
| #endif |
There was a problem hiding this comment.
Interlocked is not needed here
| #if NET6_0_OR_GREATER | |
| _ = Interlocked.Increment(ref _outboundPacketSequence); | |
| #else | |
| _outboundPacketSequence++; | |
| #endif | |
| _outboundPacketSequence++; |
| @@ -44,6 +54,31 @@ public void Send_InputStream_to_Command() | |||
| } | |||
| } | |||
|
|
|||
| #if NET6_0_OR_GREATER | |||
| [TestMethod] | |||
| [Ignore] // Not work. | |||
| _inputStream?.Close(); | ||
|
|
||
| // wait for operation to complete (or time out) | ||
| WaitOnHandle(_asyncResult.AsyncWaitHandle); |
There was a problem hiding this comment.
Yeah.. before exposing async methods on SshCommand we should probably be at the point where its implementation is natively async (without AsyncResult and its wait handle), and the Begin/EndExecute methods just wrap the native methods with TaskToAsyncResult
| throw new SshConnectionException("Client not connected."); | ||
| } | ||
|
|
||
| await SocketAbstraction.SendAsync(_socket, data, token).ConfigureAwait(false); |
There was a problem hiding this comment.
I'll clean up my branch over the next few days and add a SendAsync
I like this. We don't have a class dependency map for this project, do we? |
|
All that being said, if you want to merge this and improve the implementation from there then that's fine with me. It's up to you
Not afaik, but |
I have personally been tinkering with my own SSH library which is how I wound up contributing to this project. I can tell you from personal experience that a "rewrite" is MUCH harder than you realize -- even when using SSH.NET as a starting or reference point. Rewrites almost never succeed; they are very hard to stick with.
I think you know that I've been advocating for dropping .NET Framework 4 support, but I don't think it necessarily requires a new codebase or repo. I continue to believe that the best way to handle this is to leverage proper versioning. (Unfortunately, our versioning scheme follows a year format, so it doesn't really make it obvious and evident when we want to introduce breaking changes, i.e. hard to tell whether the new version has a new major number just because it is a new year, or new breaking changes... but anyways...) I think the simple answer is to create a TL;DR: In favor up going async all the way (yes please!); in favor of dropping .NET 4 Framework support; not in favor of rewriting from scratch or spinning up separate repo, but rather would leverage Git and proper branching to support the endeavor. P.S. - you guys are awesome. I'm a guy talking from the sidelines. Weigh that appropriately. 😄 |
|
I don't really know what would be best. A new repo or a new branch, rewrite or not, I know that if we want to move forward, it'll require breaking eggs at some point. |
|
I don't think I have anything more to say that I haven't said before. We have to remember that SSH.NET is the library for SSH/SFTP in .NET. There are many downstream projects that depend on it and which include .NET Framework in their target platforms, let alone direct users. I really think it would be inappropriate to drop support (i.e. future development) for .NET Framework and I also do not see it as a burden to future development. On the contrary, I think it would be much more burdensome to maintain separate versions. |
|
I use .NET8, and I'm the kind of guy who regularly cuts himself on the bleeding edge. Looking at the support lifecycle for Framework we can see that 4.8 only came out in 2019 and it doesn't have a listed EOL date at all. As Microsoft has no plans to drop support of framework, I don't think any other critical libraries should either. While it makes sense to drop earlier versions, 4.8 lives on eternal. |
|
@A9G-Data-Droid that is the best point I've heard so far in favor of keeping .NET Framework support. Right now we are on .NET 4.6.2. We should switch to 4.8. That may even alleviate some of these pain points. |
This is just a PoC, I wanted to check how much work is needed to implement true async. Unfortunately, it turns out that a lot. I don't know if it's worth going in this direction before giving up .NET Framework 4.
Challenges:
@Rob-Hague, @drieseng, @jacobslusser, @scott-xu, @jscarle I am open to comments on this idea. Does this make any sense at all?