Skip to content

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

Open
pvgnd wants to merge 1 commit into
thephpleague:masterfrom
pvgnd:fix-is-connected
Open

FIX: isConnected() does not detect that the server closed the connection#95
pvgnd wants to merge 1 commit into
thephpleague:masterfrom
pvgnd:fix-is-connected

Conversation

@pvgnd

@pvgnd pvgnd commented Oct 31, 2019

Copy link
Copy Markdown

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 force-pushed the fix-is-connected branch from 47ac707 to cd34e4e Compare April 25, 2020 11:02
@pvgnd
pvgnd force-pushed the fix-is-connected branch from cd34e4e to 96b0df0 Compare April 25, 2020 11:12
@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

pvgnd commented Apr 25, 2020

Copy link
Copy Markdown
Author

@frankdejonge this PR is ready for review :)

@ashokadewit

Copy link
Copy Markdown

@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
Copy Markdown
Member

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

@sylvain-duval

sylvain-duval commented Jul 8, 2021

Copy link
Copy Markdown

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

mdm88 commented Aug 3, 2021

Copy link
Copy Markdown

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

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

@olsavmic

olsavmic commented Oct 12, 2021

Copy link
Copy Markdown

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
Copy Markdown

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

@kiropowered

Copy link
Copy Markdown

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

@olsavmic

olsavmic commented Apr 18, 2022

Copy link
Copy Markdown
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
Copy Markdown

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

ghost commented Apr 19, 2022

Copy link
Copy Markdown

@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
Copy Markdown

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.

8 participants