Effective concurrent SshClient connections bumps into a limit

[cankut]

I am trying to make SSH connections to multiple clients concurrently to check their availability.

I noticed that, after a specific limit, increasing thread count doesn't have an effect on total process time.

For testing purposes,
I choose a fake IP address that connection will never happen and set the connection timeout to 5 seconds.

private static void MakeConnection()
 {
 using (var client = new SshClient("1.2.3.4", 22, "test", "test"))
 {
 client.ConnectionInfo.Timeout = TimeSpan.FromSeconds(5);
 try
 {
 client.Connect();
 }
 catch { }
 }
 }

Executed test runs for different thread & connection counts.

Here are the results:

Thread Count	Connection Count	Total Process Time
1	1	5 sec
2	2	5 sec
3	3	5 sec
4	4	10 sec
5	5	10 sec
6	6	10 sec
7	7	15 sec
8	8	15 sec

I expect to see 5 sec for each row but somehow effective active thread counts seems like 3. Even though I spawn 8 threads, it has been processed as 3+3+2.

What might be causing this behaviour?

Simple console application to reproduce the issue:
https://gist.github.com/cankut/4efb95d2da8fdf63fb14cf762b1f953d

[keithallenjackson]

look at Session class. There is a private static Semaphore with a limit of 3 concurrent connections. Changing the variable from static to an instance variable may change your performance issues.

[keithallenjackson]

After closer inspection, the Semaphore I was talking about is only for concurrent connection attempts, not on-going connections. Don't be surprised if it doesn't help. I personally can run about 32 concurrent ongoing connections on a quad-core i7 at around ~5-10% cpu usage, and logging all output to files. Most of the framework is synchronous or "non-blocking" calls used in a synchronous way. So, the more cores, the better.

[patgauvingeek]

I'm trying to connect on several devices in parallel to update their software and this semaphore is slowing the whole process. Like @cankut demonstrate, the worst case is when the servers don't reply.

I think the semaphore have to be removed completely because I think this semaphore should be at the application level, not in the library. Take a look at the remarks on the semaphore:

/// <summary>
/// Controls how many authentication attempts can take place at the same time.
/// </summary>
/// <remarks>
/// Some server may restrict number to prevent authentication attacks
/// </remarks>
private static readonly SemaphoreLight AuthenticationConnection = new SemaphoreLight(3);

Base on that comment, I think the server used to test the library has that restriction and this was the fix to make the tests work when executed in parallel. Moving the semaphore in the test (which is our application level here) gives full control to the user of the library like it should without preventing the test to work when executed in parallel.

What do you think?

To conclude, I'd like to share something that one of my colleagues taught me: It's always a good design to let the synchronization mechanism to the application level. This is the best level to do it because it knows how many thread there is and how they are handle. Only one thread? No synchronization required. It has a shared memory accessed by multiple process? You need a named mutex. Multiple threads? A semaphore will do it. etc... A library can't (and shouldn't try to) handle all those possibilities.

Cheers!

[ascott18]

This would be great if it could be fixed. When running a large number of one-off commands against a large number of targets in parallel using the .NET thread pool, the synchronous .Wait() call blocks the threads and ties up the thread pool, causing significantly degraded performance in the rest of our system's use of the thread pool until the connections start going through and clearing up the blocked thread pool threads.

[Rob-Hague]

The AuthenticationConnection semaphore has been removed in #1304

[WojciechNagorski]

This issue has been fixed in the 2024.0.0 version.