Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: How to reliably detect if SftpClient is still connected #843

Closed
IgorMilavec opened this issue Jun 30, 2021 · 16 comments · Fixed by #1362
Closed

Discussion: How to reliably detect if SftpClient is still connected #843

IgorMilavec opened this issue Jun 30, 2021 · 16 comments · Fixed by #1362

Comments

@IgorMilavec
Copy link
Collaborator

IgorMilavec commented Jun 30, 2021

I need to implement SFTP connection pooling. As part of this I need to have long running SftpClients and a way to check if the client is still connected. Initially I used SftpClient.IsConnected for this purpose. However, even if SftpClient.IsConnected returned true, I many times got this exception when trying to use the SftpClient instance:

System.InvalidOperationException: The session is not open.
   at Renci.SshNet.SubsystemSession.EnsureSessionIsOpen()
   at Renci.SshNet.SubsystemSession.SendData(Byte[] data)
   at Renci.SshNet.Sftp.SftpSession.SendMessage(SftpMessage sftpMessage)
   at Renci.SshNet.Sftp.SftpSession.SendRequest(SftpRequest request)
...

Here is the class diagram of the relevant portion of SSH.NET: https://gitmind.com/app/flowchart/75ace876ac90aec4dbce3b61347c6cf9

Based on this architecture, it seems to me that the described problem is caused by SftpClient not taking into account the SubsystemSession's state. As an intermediate workaround I made BaseClient.IsConnected virtual and implemented it in SftpClient as:

        public override bool IsConnected
        {
            get
            {
                return base.IsConnected && _sftpSession.IsOpen;
            }
        }

Initial tests show this solves my problem, but further testing is needed to really claim this.

Can someone with more insight into the codebase confirm my reasoning so far? I would also be thankful if the one could answer the questions that pop up in my mind looking at this class diagram:

  • There are a lot of classes named *Session in this diagram... can someone explain what these classes do conceptually? Is the naming good?
  • Is there a reason for Session and SubsystemSession to hold their own respective status? Does/should a change in one's state relate to a change in the other's state?
  • Why do BaseClient and Session each hold ConnectionInfo? Are these two the same?
@WolfspiritM
Copy link

That's interesting. We've been running SFTP for over a year now with pooling connections without ever getting this error (at least not noticing it) but recently since a few days ago without any update on our side we started to see some connections to fail with the "The session is not open" exception, too. We're also using IsConnected to check if the connection is still open.

@IgorMilavec
Copy link
Collaborator Author

This behavior seems to be triggered by specific configuration on the server. I only get this for some partners. I presume these partners have session timeout configured and their server disconnects the SFTP session, but does not terminate the connection.
Are you willing to try the above workaround to see if it works for you?

@avr2222
Copy link

avr2222 commented Aug 31, 2021

I am getting the same error. Any suggestions?

@IgorMilavec
Copy link
Collaborator Author

You could implement the above mentioned workaround. I haven't seen this error since I implemented it.

@avr2222
Copy link

avr2222 commented Sep 8, 2021

You mean implemeting this?
public override bool IsConnected
{
get
{
return base.IsConnected && _sftpSession.IsOpen;
}
}

@IgorMilavec
Copy link
Collaborator Author

I have pushed the required changes to https://github.com/IgorMilavec/SSH.NET/tree/Issue843 so you can do a private build.
No PR to upstream at this time as I feel this needs to be discussed further. @drieseng, any thoughts on this?

@avr2222
Copy link

avr2222 commented Sep 9, 2021

Ok fine. For now, i will try this and update you.

@anandgmenon
Copy link

This issue seems to be still relevant, can we open a fix to address this?

@WojciechNagorski
Copy link
Collaborator

Yes

@mus65
Copy link
Contributor

mus65 commented Mar 22, 2024

The proposed fix is imho not correct or at least not complete. Connect() will throw an exception if the connection is still open. That means the following code would throw an exception if the SFTP session is closed, but the connection is still open.

if (!sftpClient.IsConnected)
{
  sftpClient.Connect();
}

You would obviously not expect this to fail. Not sure what the correct approach/fix would be here, I just thought this was worth mentioning.

mus65 added a commit to mus65/SSH.NET that referenced this issue Mar 23, 2024
If the server closes the SFTP session but keeps the TCP connection open,
this currently causes IsConnected to return true, but any operation
fails with "the session is not open".

SftpClient.IsConnected now also check sftpSession.IsOpen. Connect()
and ConnectAsync() were reworked to take into account that the Session
may already/still be open, but the SFTP session may not. This is needed
so a reconnect works.

fixes sshnet#843 and sshnet#1153
WojciechNagorski pushed a commit that referenced this issue Apr 4, 2024
* SftpClient: handle the SFTP session being closed by the server

If the server closes the SFTP session but keeps the TCP connection open,
this currently causes IsConnected to return true, but any operation
fails with "the session is not open".

SftpClient.IsConnected now also check sftpSession.IsOpen. Connect()
and ConnectAsync() were reworked to take into account that the Session
may already/still be open, but the SFTP session may not. This is needed
so a reconnect works.

fixes #843 and #1153

* Always re-create session

---------

Co-authored-by: Rob Hague <[email protected]>
Co-authored-by: Igor Milavec <[email protected]>
@kev-andrews
Copy link

Am still seeing this issue in a similar connection pooling scenario. After fix #1362 we are seeing "Client not connected." after successful "IsConnected" checks originating from

throw new SshConnectionException("Client not connected.");

Is there any way IsConnected can check the socket as well?

@toblast
Copy link

toblast commented Aug 2, 2024

Am still seeing this issue in a similar connection pooling scenario. After fix #1362 we are seeing "Client not connected." after successful "IsConnected" checks originating from

throw new SshConnectionException("Client not connected.");

Is there any way IsConnected can check the socket as well?

I get the exact the same problem.

@mus65
Copy link
Contributor

mus65 commented Aug 23, 2024

@kev-andrews @toblast I created a new issue #1474 for this.

@Rob-Hague
Copy link
Collaborator

It seems to me that BaseClient already checks the socket (with SelectRead instead of SelectWrite but I'm not sure that matters). It also has KeepAliveInterval which when set will send packets on the wire. Sounds to me like the most "reliable" thing if we ignore the inherent race condition

@mus65
Copy link
Contributor

mus65 commented Aug 24, 2024

I think you're right, setting a keep alive is the only way to properly avoid an exception. Closed #1474 .

@mus65
Copy link
Contributor

mus65 commented Sep 6, 2024

For reference: there was a bug here after all which should be fixed with next release (see #1474).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants