Repository navigation
AttributeError: 'SFTPHook' object has no attribute 'close_conn' #47201
Description
Activity
Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.
(and add a missing unit test apparently)
I'm on it will try to fix it asap
Duplicate of #47129 ?
Yes. I think this will continue to break for the users depending upon how they are using it at the moment. For example we have a wrapper around SFTPHook that will allow us to do multiple interactions with SFTP server with one open connection. For instance, check if certain file matches the pattern, send the file to different server(and additionally create directory on the destination server) and then delete the file from source.
def my_file_transfer_function(): srcConnectionWrapper = FTPConnection(source) destConnectionWrapper = FTPConnection(dest) srcConnectionWrapper.file_exsist(src_file_path) srcConnectionWrapper.sftp_client.getfo(src_file_path, destConnectionWrapper.sftp_client.open( dest_file_path, 'wb')) srcConnectionWrapper.delete_file(src_file_path)So now we need to open three connections to source and 2 connections to destination server if we were to do the same using 5.1.0. I know this is very specific implementation but I am trying to say that we can have all kinds of implementation which could break if we change the get_conn() in sftphook
Duplicate of #47129 ?
Yes. I think this will continue to break for the users depending upon how they are using it at the moment. For example we have a wrapper around SFTPHook that will allow us to do multiple interactions with SFTP server with one open connection. For instance, check if certain file matches the pattern, send the file to different server(and additionally create directory on the destination server) and then delete the file from source.
def my_file_transfer_function(): srcConnectionWrapper = FTPConnection(source) destConnectionWrapper = FTPConnection(dest) srcConnectionWrapper.file_exsist(src_file_path) srcConnectionWrapper.sftp_client.getfo(src_file_path, destConnectionWrapper.sftp_client.open( dest_file_path, 'wb')) srcConnectionWrapper.delete_file(src_file_path)So now we need to open three connections to source and 2 connections to destination server if we were to do the same using 5.1.0. I know this is very specific implementation but I am trying to say that we can have all kinds of implementation which could break if we change the get_conn() in sftphook
The reason why this PR changed the implementation was that we discovered connection leaking due to the caching of the connection in the SFTPHook. Also even if the connection is closed using the close_conn method of the SFTPHook, the underlying SSHClient isn't closed and caused connections leaks. In you example above, I would then opt to get the context managed connection and call the underlying SFTPClient methods directly.
Duplicate of #47129 ?
Yes. I think this will continue to break for the users depending upon how they are using it at the moment. For example we have a wrapper around SFTPHook that will allow us to do multiple interactions with SFTP server with one open connection. For instance, check if certain file matches the pattern, send the file to different server(and additionally create directory on the destination server) and then delete the file from source.
def my_file_transfer_function(): srcConnectionWrapper = FTPConnection(source) destConnectionWrapper = FTPConnection(dest) srcConnectionWrapper.file_exsist(src_file_path) srcConnectionWrapper.sftp_client.getfo(src_file_path, destConnectionWrapper.sftp_client.open( dest_file_path, 'wb')) srcConnectionWrapper.delete_file(src_file_path)So now we need to open three connections to source and 2 connections to destination server if we were to do the same using 5.1.0. I know this is very specific implementation but I am trying to say that we can have all kinds of implementation which could break if we change the get_conn() in sftphook
The reason why this PR changed the implementation was that we discovered connection leaking due to the caching of the connection in the SFTPHook. Also even if the connection is closed using the close_conn method of the SFTPHook, the underlying SSHClient isn't closed and caused connections leaks. In you example above, I would then opt to get the context managed connection and call the underlying SFTPClient methods directly.
While I understand the need for the change, I am just thinking if there could be any other way of providing this functionality instead of enforcing to change the existing implementation.
Does it really not close the underlying connection even if we call the close method that it is offering right now? Although i did not showcase in my code, we do use that method to close the connection once we are done with all the operations.
I mean If SFTPHook.close_conn which calls the underlying close function of paramiko then how adding a closing statement solves it? If I i am not wrong the closing function from contextlib will do the same thing right?
Reacted by David BlainDuplicate of #47129 ?
Yes. I think this will continue to break for the users depending upon how they are using it at the moment. For example we have a wrapper around SFTPHook that will allow us to do multiple interactions with SFTP server with one open connection. For instance, check if certain file matches the pattern, send the file to different server(and additionally create directory on the destination server) and then delete the file from source.
def my_file_transfer_function(): srcConnectionWrapper = FTPConnection(source) destConnectionWrapper = FTPConnection(dest) srcConnectionWrapper.file_exsist(src_file_path) srcConnectionWrapper.sftp_client.getfo(src_file_path, destConnectionWrapper.sftp_client.open( dest_file_path, 'wb')) srcConnectionWrapper.delete_file(src_file_path)So now we need to open three connections to source and 2 connections to destination server if we were to do the same using 5.1.0. I know this is very specific implementation but I am trying to say that we can have all kinds of implementation which could break if we change the get_conn() in sftphook
The reason why this PR changed the implementation was that we discovered connection leaking due to the caching of the connection in the SFTPHook. Also even if the connection is closed using the close_conn method of the SFTPHook, the underlying SSHClient isn't closed and caused connections leaks. In you example above, I would then opt to get the context managed connection and call the underlying SFTPClient methods directly.
While I understand the need for the change, I am just thinking if there could be any other way of providing this functionality instead of enforcing to change the existing implementation.
Does it really not close the underlying connection even if we call the close method that it is offering right now? Although i did not showcase in my code, we do use that method to close the connection once we are done with all the operations.
I mean If SFTPHook.close_conn which calls the underlying close function of paramiko then how adding a closing statement solves it? If I i am not wrong the closing function from contextlib will do the same thing right?
The original close_conn only closes the SFTPClient, not the SSHClient, hence why the new implementation had 2 closing statements, one for SFTPClient and one for SSHClient. Closing the SFTPClient doesn't close the SSHClient, which ultimately the FTP server we connected to was out of connections after a while. We used the SFTPHook in multi threaded environment with lots of connections, which where leaking. The PR I've made solved that issue, as we didn't experience connection leaking after the fix.
Also keeping an open connection cached in a Hook is dangerous imho, as you're never certain how the caller will behave, you don't know how the method is going to be used outside thus can also lead to cached connections being kept open, while the one with the context manager (it has now be renamed to get_managed_conn, orginal get_conn method is back in PR), you're certain the connection will be closed once out of scope.
Also we didn't not only experience connection leaking with the SFTPHook, but also with the SFTPOperator, because first I though the issue was in the operator, but after some testing, I discovered the leak was from the SFTPHook and the way get_conn was implemented. You can check the operator code, the cached connection from get_conn is never closed, which means SFTPClient as well as SSHClient stay open.
Reacted by Hanuma Vivek Meka
Apache Airflow Provider(s)
sftp
Versions of Apache Airflow Providers
The latest version of apache-airflow-providers-sftp (5.1.0) has encountered an issue due to the removal of the close_conn() function in a recent commit.(fbd19da#diff-d12bfdf9541f6eaf52829b1040a95cc70d8e91a78d3eebe2771232d8ba5d7888L102-L106)
The SFTP sensor continues to invoke the close_conn() function, resulting in an error.(https://github.com/apache/airflow/blob/main/providers/sftp/src/airflow/providers/sftp/sensors/sftp.py#L132)
PS:
I have just downgraded the package to version 5.0.0, and it is functioning properly.
Apache Airflow version
2.9.1
Operating System
linux
Deployment
Google Cloud Composer
Deployment details
No response
What happened
No response
What you think should happen instead
No response
How to reproduce
Install apache-airflow-providers-sftp 5.1.0
`
from airflow.models import DAG
from airflow.providers.sftp.sensors.sftp import SFTPSensor
from airflow.operators.empty import EmptyOperator
from airflow.providers.sftp.operators.sftp import SFTPOperator
with DAG(
"airflow_file_trigger_poc",
schedule_interval=None
) as dag:
`
If you run this, you will find the error.
Anything else
No response
Are you willing to submit PR?
Code of Conduct