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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pvgnd
Copy link

@pvgnd pvgnd commented Oct 31, 2019

No description provided.

@pvgnd pvgnd changed the title FIX: isConnected() does not detect that the server closes the connection WIP: FIX: isConnected() does not detect that the server closes the connection Oct 31, 2019
@pvgnd pvgnd changed the title WIP: FIX: isConnected() does not detect that the server closes the connection WIP: FIX: isConnected() does not detect that the server closed the connection Oct 31, 2019
@pvgnd pvgnd changed the title WIP: FIX: isConnected() does not detect that the server closed the connection FIX: isConnected() does not detect that the server closed the connection Apr 25, 2020
@pvgnd
Copy link
Author

pvgnd commented Apr 25, 2020

@frankdejonge this PR is ready for review :)

@ashokadewit
Copy link

@frankdejonge Any chance this is going to get merged? This problem was described in #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.

@frankdejonge
Copy link
Member

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

@sylvain-duval
Copy link

sylvain-duval commented Jul 8, 2021

Hello guys,
We are in the same situation as the one described here : #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.

@mdm88
Copy link

mdm88 commented Aug 3, 2021

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

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

@olsavmic
Copy link

olsavmic commented Oct 12, 2021

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.

@sylvain-duval
Copy link

Hello it seems like @frankdejonge worked on it can't wait for that feature thanks man
e1e8afa

@kiropowered
Copy link

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

@olsavmic
Copy link

olsavmic commented Apr 18, 2022

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

@kiropowered
Copy link

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

@ghost
Copy link

ghost commented Apr 19, 2022

@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.

@jeff1326
Copy link

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.

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 this pull request may close these issues.

None yet

8 participants