Connections in long running processes time out, but isConnected() stays true

[sebsel]

This is probably related to #1126 and thus #985.

A more detailed description of my problem can be found in this issue on the Flysystem SftpAdapter, which uses this library to provide easy sFTP access:
thephpleague/flysystem-sftp#66

The main point is that, when used in a long running process, a SSH connection can drop due to inactivity. Before every attempt, Flysystem checks the connection. The Flysystem adapter fails to reconnect, because ->isConnected() (in \phpseclib\Net\SFTP which inherits the method from \phpseclib\Net\SSH2) still returns true after the connection has timed out.

I don't think sending a ping (like suggested in #1126) would be a good solution to this. If there is some way to detect the disconnection in ->isConnected(), the problem would be solved already.

[terrafrost]

I think something that would be better would be something like adding $this->bitmap = 0; every time user_error('Connection closed by server'); is called. That's what ->isConnected() looks at.

Another possibility. You could do something like this (this is for phpseclib 1.0 but can be easily adapted to work on phpseclib 2.0):

<?php
include('Net/SSH2.php');

define('NET_SSH2_CHANNEL_KEEP_ALIVE', 100);

class Net_SSH2_Ping extends Net_SSH2 {
    /**
     * Authentication Credentials
     *
     * @var array
     * @access private
     */
    var $auth = array();

    /**
     * Login
     *
     * The $password parameter can be a plaintext password, a Crypt_RSA object or an array
     *
     * @param string $username
     * @param mixed $password
     * @param mixed $...
     * @return bool
     * @see self::_login()
     * @access public
     */
    function login($username)
    {
        $args = func_get_args();
        $this->auth[] = $args;
        return call_user_func_array(array(&$this, 'parent::login'), $args);
    }

    /**
     * Pings a server connection, or tries to reconnect if the connection has gone down
     *
     * Inspired by http://php.net/manual/en/mysqli.ping.php
     *
     * @return string
     * @access public
     */
    function ping()
    {
        if (!$this->isAuthenticated()) {
            return false;
        }

        $this->window_size_server_to_client[NET_SSH2_CHANNEL_KEEP_ALIVE] = $this->window_size;
        $packet_size = 0x4000;
        $packet = pack(
            'CNa*N3',
            NET_SSH2_MSG_CHANNEL_OPEN,
            strlen('session'),
            'session',
            NET_SSH2_CHANNEL_KEEP_ALIVE,
            $this->window_size_server_to_client[NET_SSH2_CHANNEL_KEEP_ALIVE],
            $packet_size
        );

        if (!@$this->_send_binary_packet($packet)) {
            return $this->_reconnect();
        }

        $this->channel_status[NET_SSH2_CHANNEL_KEEP_ALIVE] = NET_SSH2_MSG_CHANNEL_OPEN;

        $response = @$this->_get_channel_packet(NET_SSH2_CHANNEL_KEEP_ALIVE);
        if ($response !== false) {
            $this->_close_channel(NET_SSH2_CHANNEL_KEEP_ALIVE);
            return true;
        }

        return $this->_reconnect();
    }

    /**
     * In situ reconnect method
     *
     * @return boolean
     * @access private
     */
    function _reconnect()
    {
        $this->_reset_connection(NET_SSH2_DISCONNECT_CONNECTION_LOST);
        $this->retry_connect = true;
        if (!$this->_connect()) {
            return false;
        }
        foreach ($this->auth as $auth) {
            $result = call_user_func_array(array(&$this, 'parent::login'), $auth);
        }
        return $result;
    }
}

This is inspired by mysqli_ping. I'm not sure what I think of saving the credentials like this does. Also, any state changes will be lost. Like let's say you were in the middle of an interactive session. If this reconnected you you would no longer be in that interactive session. I suppose this is just as much an issue with mysqli_ping as it is with this method, however, since, with mysqli_ping, you could be in the middle of an SQL TRANSACTION.

Also, FWIW, SSH doesn't really have a packet that does exactly what one would expect from a keep-alive packet. A keep-alive packet would normally be... you send a request and you get a response and that's it. With this implementation you send a request, get a response, then send another two request and then get another response. eg.

 -> NET_SSH2_MSG_CHANNEL_OPEN (since last: 0.0149, network: 0.0001s)
00000000  00:00:00:07:73:65:73:73:69:6f:6e:00:00:00:64:7f  ....session...d.
00000010  ff:ff:ff:00:00:40:00                             .....@.

<- NET_SSH2_MSG_CHANNEL_OPEN_CONFIRMATION (since last: 0.0223, network: 0.0001s)
00000000  00:00:00:64:00:00:00:00:00:00:00:00:00:00:80:00  ...d............

-> NET_SSH2_MSG_CHANNEL_EOF (since last: 0.0079, network: 0.0002s)
00000000  00:00:00:00                                      ....

-> NET_SSH2_MSG_CHANNEL_CLOSE (since last: 0.0341, network: 0.0003s)
00000000  00:00:00:00                                      ....

<- NET_SSH2_MSG_CHANNEL_CLOSE (since last: 0.0319, network: 0.0001s)
00000000  00:00:00:64                                      ...d

Technically, I think that could probably be reduced down to having to only send two requests instead of sending three but that's the general idea.

[terrafrost]

I think something that would be better would be something like adding $this->bitmap = 0; every time user_error('Connection closed by server'); is called. That's what ->isConnected() looks at.

That's been done. Now when you get a "Connection closed by server" error ->isConnected() should return false.

I also implemented the ping method I proposed.

Thanks!

[sebsel]

Nice, thanks! Is there a release with these fixes coming up? :)

[terrafrost]

I'll try to do a release this weekend or next.

Thanks!

[terrafrost]

I just made a new release.

Thanks!

[sebsel]

I'm afraid I still found a bug with this. :(

You missed a few spots and my use-case hit one of them:
#1313

[sebsel]

Just for completeness: the new release indeed fixed the problem.

[Echron]

@terrafrost I know this is a closed issue but just in case I missed something before opening a new ticket:
I got warnings when running the ping function with PHP 7.1 that the constant NET_SSH2_CHANNEL_KEEP_ALIVE is not defined. I see in your example that you define it outside the class, but I don't see the defining of the constant in the SSH2 class. I now define it before using the SSH2. Is this how you see it work, do I imply something wrong or should this get fixed?

[terrafrost]

@Echron - I think e5ff894 should fix this.

Thanks!