thephpleague / flysystem

Abstraction for local and remote filesystems
https://flysystem.thephpleague.com
MIT License
13.33k stars 825 forks source link

SFTP connections - do we need to disconnect each time? #1660

Closed geebeetoo closed 1 year ago

geebeetoo commented 1 year ago

Question

Q A
Flysystem Version flysystem-sftp-v3
Adapter Name example
Adapter version x.y.z

When we submit several jobs via laravel queue, eventually after about 150 jobs we get "unable to access" - would that be because there are too many connections?

Is there such a thing as closing or disconnecting connections after every job?

Thanks

frankdejonge commented 1 year ago

The issue does not list the required fields, nor does it share enough info to investigate.

There is a ConnectionProvider interface which allows you to control how a connection is established. You can use that to implement any connection/re-connect logic you may need.

ttruong15 commented 8 months ago

When the queue worker run it does not kill the established connection. If there's 4 workers then there will be 4 established connection stay active. Depending on the server if they have a set timeout then you next call outside that windows will failed. So if you try to connect to the server to receive some file you might get something like this

shallow listing Reason: Connection closed by server

So my solution is to call disconnect explicitly everytime. $sftp = Storage::disk('sftp'); $gotSomeFiles = $sftp->files('data'); $sftp->getAdapter()->getProvideConnection()->disconnect();

There's a ping() method you can use. The ping will check the connection and re-established if necessary but once the job is done the connection still active. I prefer to use disconnect so I know there's no stale connection floating around taking up resources.

frankdejonge commented 8 months ago

@ttruong15 there is an interface you can implement for custom connectivity checking logic: src/PhpseclibV2/ConnectivityChecker.php

ttruong15 commented 8 months ago

@frankdejonge thanks that does work but it doesn't meet my needs. That only check if the connection is active or not and then reconnect again. However that will always leave the connection active.

The Laravel queue worker when run via supervisor doesn't get kill so it never get to call the __desctruct() of the SSH2 so it can clean up the connection. I have 4 workers which means there's 4 connections sitting idle wasting resources.

I want to be able to connect to the server, do what i need to do and disconnect and clean up the connection. Which means every time I access the server it have to reconnect/login again which I am fine with it.

Anyway, I think I solved my issue. thanks for your help.,

Btw, if someone want to implement the connectivity checker the config need to be this sftp => [ ... 'connectivityChecker' => new MyConnectivityChecker() ]

hasfoug commented 7 months ago

@ttruong15 i have the same issue, how did you resolve it for you?

Since both connectionProvider in SftpAdapter and connection in SftpConnectionProvider are private there is no way to call disconnect on connection.

@frankdejonge can we have methods pulic methods getConnectionProvider() in SftpAdapter and getConnection() in SftpConnectionProvider? This would provide flexibility to handle cases like this one when low-level stuff needs to be done (e.g. closing a connection)

ttruong15 commented 7 months ago

Hey @hasfoug unfortunately I am back to square one. I though I had it sorted with this $sftp->getAdapter()->getProvideConnection()->disconnect(); however later version the getProvideConnection method have been removed which break backward compatibility. I know the disconnect in the phpseclib is public method, so I am not sure why we not exposing it in this library as now there's no way to directly access it. The implementation suggested by @frankdejonge does not solve my issue as that's only checking for connectivity and reconnect again. So the only way to add this public method back is maybe fork this project off and add it back yourself.

frankdejonge commented 7 months ago

@ttruong15 just so I understand your case better, and possibly can help find a solution. I assume you do not want to reconnect on every job, but when there are no jobs you want to disconnect? Your own connection provider can expose a disconnect method, and the sftp config can resolve it as a service. This means other logic in your consumer can also get that dependency and call the disconnect method when appropriate. The thing is, flysystem does not really concern itself with these concerns much but does provide injection points to add logic like this. That said, I'd be open for a PR to add a disconnect method on the connection provider. It'd need to be a non-BC breaking addition though. I can set it up too after work. But again, resolve the connection provider through a shared dependency and calling the disconnect on your custom implementation is something that works already and is quite straight forward to implement.

hasfoug commented 7 months ago

@frankdejonge its a bit different, we need to close connection when job is done writing to the SFTP server, since the connection stays open and server blocks futher connections.

Its also described here: https://github.com/thephpleague/flysystem/issues/1757

I opened a PR with disconnect method: https://github.com/thephpleague/flysystem/pull/1758/files/2f2362f44b1741bb58bee601905d273e59cb3cd4