Improper scoping keeps the thread waiting

[SwarajKetan]

File : Session.cs
Method: Connect()
Due to improper scope AuthenticationConnection semaphore lock is acquired more than once when connect() method is called more than once while a connect operation is still in progress. It exhausts the semaphore slot and keeps the other Connect operations waiting until one finishes.

[IgorMilavec]

Can you please elaborate on what you mean with "improper scope"?

[SwarajKetan]

Currently:
Scenario -1: Suppose, A thread enters the try block. Then it acquires the lock. Suppose the the session is already connected. It returns releasing lock from finally block. ---- This exhausts a semaphore slot unnecessarily.

Scenario -2: Suppose one thread is already working on connecting the socket. During that time, another thread enters the try block acquiring a lock on semaphore and waiting on 'lock(this)' ---- This exhausts a semaphore slot during the entire time when the previous thread is working on establishing connection,

In contrast, the new arrangement, semaphore is acquired just before the connection attempt. This seems to be optimized,

[SwarajKetan]

Here is the PR link:

https://github.com/sshnet/SSH.NET/pull/860/files

[IgorMilavec]

As a general comment, please note that AuthenticationConnection is a static/shared, while Connect is locking on specific object instance (this).

In scenario 1 the semaphore will be released in finally block.
Scenario 2 would only happen if Connect on the same Session instance would be called concurrently from two threads. And at least 4 threads would need to tango for this to have any impact (due to semaphore count being 3).

AFAIK the Session class' documentation doesn't state that it is thread safe. I cannot see a scenario in which this would be desirable/useful/needed. The current implementation tries to be thread safe, but does so at the expense of concurrency as you noted. While I do agree that your PR might help in some rare cases, I would propose to approach this problem from a different angle. I believe the semaphore was added to limit concurrent connection attempts to one server. Concerns such as this are much better handled by dedicated libraries, a notable example being Polly. So instead of imposing a global concurrency limit in a generic library (SSH.NET), this should be handled in the specific application having specific problems.

Of course any such change would need to be thoroughly discussed as it could break some applications.

[Rob-Hague]

The AuthenticationConnection semaphore has been removed in #1304

[WojciechNagorski]

This issue has been fixed in the 2024.0.0 version.