flysystem-sftp icon indicating copy to clipboard operation
flysystem-sftp copied to clipboard

FIX: isConnected() does not detect that the server closed the connection

Open pvgnd opened this issue 5 years ago • 12 comments

pvgnd avatar Oct 31 '19 16:10 pvgnd

@frankdejonge this PR is ready for review :)

pvgnd avatar Apr 25 '20 11:04 pvgnd

@frankdejonge Any chance this is going to get merged? This problem was described in https://github.com/thephpleague/flysystem-sftp/issues/66 but that issue was closed while the problem still exists.

In phpseclib the connection is only marked as closed after an operation has failed. So when iterating over a list of files and calling has the result of the call will be false when the connection has gone away. The connection will be opened the next time has is called. This problem leads to files incorrectly being marked as not existing.

The solution of calling ping to check if a connection is open is very similar to the FTP adapter which sends a NOOP package in the isConnected method.

This PR is for v1 though the same problem exists in v2 when using the simple connectivity checker.

ashokadewit avatar Jul 02 '21 07:07 ashokadewit

This PR is on my list for this weekend to review.

frankdejonge avatar Jul 02 '21 13:07 frankdejonge

Hello guys, We are in the same situation as the one described here : https://github.com/thephpleague/flysystem-sftp/issues/66. Our worker(long running process) is handling messages that upload files on a sftp server, after some time it get disconnected generating an error then the message is queued again for processing, after 2-3 retries it finally get uploaded. Do you think that PR will fix the problem ? Thank you very much for your work and the amazing job you do on the different the php league libraries.

sylvain-duval avatar Jul 08 '21 16:07 sylvain-duval

This PR is on my list for this weekend to review.

Hi, any news on this? I'm having the same problem

mdm88 avatar Aug 03 '21 16:08 mdm88

We actually had to implement our own retry layer due to this issue. @frankdejonge Any problems with this solution? I can rebase it on v2.

olsavmic avatar Oct 12 '21 07:10 olsavmic

Hello it seems like @frankdejonge worked on it can't wait for that feature thanks man https://github.com/thephpleague/flysystem-sftp/commit/e1e8afa6aaad5faa582f6373ad71dcd8367a1893

sylvain-duval avatar Jan 07 '22 14:01 sylvain-duval

Hi, I just had the issue, does the fix will be available soon ? @olsavmic Can you share the workaround you used pls ?

kiropowered avatar Apr 18 '22 21:04 kiropowered

use League\Flysystem\PhpseclibV2\ConnectivityChecker;
use phpseclib\Net\SFTP;

class SftpPingConnectivityChecker implements ConnectivityChecker
{

    public function isConnected(SFTP $connection): bool
    {
        return $connection->ping() && $connection->isConnected();
    }

}

@kiropowered We just register our own connectivity checker that performs pinging. Depends on your project/configuration but if you're by chance using oneup/flysystem-bundle, the connectivity checker can be overridden directly in the configuration.

oneup_flysystem:
  adapters:
    sftp_adapter:
      sftp:
        options:
          connectivityChecker: Fulfillment\CommonBundle\SFTP\SftpPingConnectivityChecker

olsavmic avatar Apr 18 '22 21:04 olsavmic

Thank you for the answer. That's what I tried to do, unfortunatly I used the league/flysystem-bundle wich does not allow to override the connectivityChecker in the config(option currently not available). Sad the "official" bundle does not support that override, I will probably switch to this bundle thank you again

kiropowered avatar Apr 18 '22 21:04 kiropowered

@kiropowered check this out https://stackoverflow.com/questions/70436411/how-to-override-a-file-in-vendor-directory we're using Laravel so we could not directly override the class, but using this method with Composer did.

ghost avatar Apr 19 '22 06:04 ghost

This Pr could be closed since this fonctions has been implemented.

This is usable like this on Laravel 6+

'my_sftp_disk' => [
            'driver' => 'sftp',
            'host' => env('MY_SFTP_DISK_HOST'),
            'username' => env('MY_SFTP_DISK_USERNAME'),
            'password' => env('MY_SFTP_DISK_PASSWORD'),
            'usePingForConnectivityCheck' => true,
],

Thanks @pvgnd for your contribution.

jeff1326 avatar Nov 23 '23 16:11 jeff1326