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

Async PoC #1318

Closed WojciechNagorski closed 3 months ago

WojciechNagorski commented 7 months ago

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?

WojciechNagorski commented 7 months ago
Performance and allocation look similar: Method Mean Error StdDev Allocated
Connect 48.743 ms 0.9689 ms 1.9125 ms 140.96 KB
ConnectAsync 48.954 ms 0.9624 ms 1.5812 ms 142.76 KB
ConnectAndRunCommand 106.295 ms 2.0102 ms 4.0145 ms 156.81 KB
ConnectAndRunCommandAsync 106.978 ms 1.9669 ms 3.1762 ms 158.36 KB
RunCommand 8.702 ms 0.1723 ms 0.2359 ms 77.5 KB
RunCommandAsync 8.658 ms 0.1698 ms 0.1955 ms 45.71 KB
jscarle commented 7 months ago

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.

WojciechNagorski commented 7 months ago

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

Rob-Hague commented 7 months ago

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.

jscarle commented 7 months ago

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):

image

We could separate out the base protocol classes from the encryption, SFTP, SCP, and so on.

jscarle commented 7 months ago

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.

jscarle commented 7 months ago

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

I like this. We don't have a class dependency map for this project, do we?

Rob-Hague commented 7 months ago

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

We don't have a class dependency map for this project, do we?

Not afaik, but Session is the heart of it and the rest kind of just follows from RFC 4254

jacobslusser commented 7 months ago

@jscarle

I think there'd be a lot of benefit in starting a new repository and separating things into various smaller projects,

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.

@WojciechNagorski

I am open to comments on this idea. Does this make any sense at all?

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 release/* branch for the "legacy" .NET 4 framework support. e.g. the release/2024 branch. The ONLY thing that would go to that branch are critical, security fixes -- which would also be included in master/main -- leaving master/main to develop the future async version and build on all the history, tests, and work that exist. We would effectively say v2024.*.* is the last non-async version, and .NET 4 framework supported version, and only gets critical security updates. Main/master would drop all the conditional preprocessor directives and support only .NET 6+.

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

jscarle commented 7 months ago

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.

Rob-Hague commented 7 months ago

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.

A9G-Data-Droid commented 7 months ago

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.

jacobslusser commented 7 months ago

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

WojciechNagorski commented 3 months ago

I'm closing because of conflict.