[Twisted-Python] Retry: Conch Issues
I am having troubles with Conch. I've been able to reproduce them in a unit test and all the code is here along with requirements.txt: https://github.com/radifalco/conch_issues I'm trying to reuse my connection, but if it is idle too long the server shuts it down. What I don't understand is why the next call to `FileTransferClient` does not call back or errback? It will just hang forever even though the Factory and Transport know the connection was closed (as evidenced by the log messages). I don't ever want to hang, if I use FileTransferClient I would like it always call a callback or errback. I feel like Conch is missing a deferred somewhere but damned if I can find it. Thanks!
Hi On Fri, 25 Sep 2020 at 00:15, Robert DiFalco <robert.difalco@gmail.com> wrote:
I am having troubles with Conch. I've been able to reproduce them in a unit test and all the code is here along with requirements.txt: https://github.com/radifalco/conch_issues
I'm trying to reuse my connection, but if it is idle too long the server shuts it down. What I don't understand is why the next call to `FileTransferClient` does not call back or errback? It will just hang forever even though the Factory and Transport know the connection was closed (as evidenced by the log messages).
I don't ever want to hang, if I use FileTransferClient I would like it always call a callback or errback. I feel like Conch is missing a deferred somewhere but damned if I can find it.
Thanks!
Have you checked https://twistedmatrix.com/documents/current/core/howto/clients.html#reconnec... ?
I see that `connector.connect()` is commented in your code. The theory is that once a client connection is lost, you will need to call again the whole chain starting with reactor.connectTCP. --------- I know the pain here with SFTP as there is a lot of wrapping: TCP client, SSH high level protocol, SSH session, SSH channel, SSH subsystem. One option is to use MySSHClientFactory as a proxy for any SFTP operation. When the SSH channel is open and you have received the response from the subsystem request, pass the SFTP client all the way down to the factory. The factory will know when the connection is lost and then trigger a new connection... which will update the SFTP client instance on the factory. That is, instead of getting the SFTP client, the SFTP client will be pushed to the factory. I think that the main issue with your code is that it tries to work directly with the SFTP client, instead of using the factory as a proxy for SFTP operations. A protocol represents a single connection that is already made and it can't reconnect itself. The factory is responsible for handling new connections, or triggering new connections. So, your high-level API is using a very low-level object. The SFTP client represents an SSH connection that was already made, authenticated and the SFTP subsystem requested. For my production code I am doing something like this: MySSHClientFactory is instantiated with the reactor and the SFTPClientOptions. Then, when I want to list a directory I call sftp_factory.listDir('some/path') The factory can then check if it needs to get an SFTP client or reuse an existing one. A SFTP client is obtained by triggering a new connectTCP to the reactor with `self` as the factory. Does it make sense? -- Adi Roiban
Thank you so much Aldi! Yes there are so many little decoupled pieces it's difficult to figre out how to use them in concert. .Let's leave aside the reconnecting option for a minute and just talk about how to percolate errors up. Because currently I want to be able to always return from a FileTransferClient call. By upleveling things do you mean like this? Wait, ha, I actually don't know because the deferreds are one shot? Why exactly is makeDirectory not firing any callbacks or errbacks and what can I add at the Factory level to make it do so? class MySSHClientFactory(SSHClientFactory): def clientConnectionLost(self, connector, reason): self.op.addErrback(reason) def makeDirectory(self, path, attrs): self.op = Deferred() def _cbSuccess(result): self.op.callback(result) return self.sftpClient.makeDirectory(path, attrs).addCallback(_cbSuccess)d That won't work. If you can give me a breadcrumb or idiom I can apply it across everything. I'm just too new to twisted to know the correct idiom here. Totally good with moving the methods up to the factory, just not sure how. On Thu, Sep 24, 2020 at 5:36 PM Adi Roiban <adi@roiban.ro> wrote:
Hi
On Fri, 25 Sep 2020 at 00:15, Robert DiFalco <robert.difalco@gmail.com> wrote:
I am having troubles with Conch. I've been able to reproduce them in a unit test and all the code is here along with requirements.txt: https://github.com/radifalco/conch_issues
I'm trying to reuse my connection, but if it is idle too long the server shuts it down. What I don't understand is why the next call to `FileTransferClient` does not call back or errback? It will just hang forever even though the Factory and Transport know the connection was closed (as evidenced by the log messages).
I don't ever want to hang, if I use FileTransferClient I would like it always call a callback or errback. I feel like Conch is missing a deferred somewhere but damned if I can find it.
Thanks!
Have you checked https://twistedmatrix.com/documents/current/core/howto/clients.html#reconnec... ?
I see that `connector.connect()` is commented in your code.
The theory is that once a client connection is lost, you will need to call again the whole chain starting with reactor.connectTCP.
---------
I know the pain here with SFTP as there is a lot of wrapping: TCP client, SSH high level protocol, SSH session, SSH channel, SSH subsystem.
One option is to use MySSHClientFactory as a proxy for any SFTP operation. When the SSH channel is open and you have received the response from the subsystem request, pass the SFTP client all the way down to the factory. The factory will know when the connection is lost and then trigger a new connection... which will update the SFTP client instance on the factory.
That is, instead of getting the SFTP client, the SFTP client will be pushed to the factory.
I think that the main issue with your code is that it tries to work directly with the SFTP client, instead of using the factory as a proxy for SFTP operations.
A protocol represents a single connection that is already made and it can't reconnect itself. The factory is responsible for handling new connections, or triggering new connections.
So, your high-level API is using a very low-level object. The SFTP client represents an SSH connection that was already made, authenticated and the SFTP subsystem requested.
For my production code I am doing something like this: MySSHClientFactory is instantiated with the reactor and the SFTPClientOptions. Then, when I want to list a directory I call sftp_factory.listDir('some/path') The factory can then check if it needs to get an SFTP client or reuse an existing one. A SFTP client is obtained by triggering a new connectTCP to the reactor with `self` as the factory.
Does it make sense?
-- Adi Roiban _______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
On Thu, Sep 24, 2020 at 8:54 PM Robert DiFalco <robert.difalco@gmail.com> wrote:
Why exactly is makeDirectory not firing any callbacks or errbacks and what can I add at the Factory level to make it do so?
class MySSHClientFactory(SSHClientFactory):
def clientConnectionLost(self, connector, reason): self.op.addErrback(reason)
def makeDirectory(self, path, attrs): self.op = Deferred() def _cbSuccess(result): self.op.callback(result)
return self.sftpClient.makeDirectory(path, attrs).addCallback(_cbSuccess)d
This code is wrong. It takes what *may* be a perfectly good Deferred from makeDirectory and breaks the error chain off of it. The correct way to do this would be more like `makeDirectory(...).chainDeferred(self.op)`. However, `self.op` and `_cbSuccess` are doing nothing useful here so you may as well just delete all that and return the `makeDirectory(...)` Deferred. If *that* Deferred doesn't fire its errback chain when the connection is closed, that's a bug in Conch. If you can put together a minimal example and file a ticket, that would be wonderful and perhaps someone can get it fixed for you. Jean-Paul
I agree it's a bug in conch and I believe my repo shows a reproducible example. Run the test, it never completes because the deferred from `makeDirectory` never fires. . On Fri, Sep 25, 2020 at 3:41 AM Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
On Thu, Sep 24, 2020 at 8:54 PM Robert DiFalco <robert.difalco@gmail.com> wrote:
Why exactly is makeDirectory not firing any callbacks or errbacks and what can I add at the Factory level to make it do so?
class MySSHClientFactory(SSHClientFactory):
def clientConnectionLost(self, connector, reason): self.op.addErrback(reason)
def makeDirectory(self, path, attrs): self.op = Deferred() def _cbSuccess(result): self.op.callback(result)
return self.sftpClient.makeDirectory(path, attrs).addCallback(_cbSuccess)d
This code is wrong. It takes what *may* be a perfectly good Deferred from makeDirectory and breaks the error chain off of it. The correct way to do this would be more like `makeDirectory(...).chainDeferred(self.op)`. However, `self.op` and `_cbSuccess` are doing nothing useful here so you may as well just delete all that and return the `makeDirectory(...)` Deferred.
If *that* Deferred doesn't fire its errback chain when the connection is closed, that's a bug in Conch. If you can put together a minimal example and file a ticket, that would be wonderful and perhaps someone can get it fixed for you.
Jean-Paul _______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
My work around for the twisted/conch bug if anyone is interested. FileTransferHandler was not handling closed connections properly. Btw, this is with release version 20.3.0, which seems to be the latest version. However, if I look in the Twisted repo it looks like someone is trying to address this bug for the next release. https://github.com/radifalco/conch_issues/commit/c25aa6bca20201f51d59c3a6dca4c1fb2aed2242?branch=c25aa6bca20201f51d59c3a6dca4c1fb2aed2242&diff=split On Fri, Sep 25, 2020 at 7:42 AM Robert DiFalco <robert.difalco@gmail.com> wrote:
I agree it's a bug in conch and I believe my repo shows a reproducible example. Run the test, it never completes because the deferred from `makeDirectory` never fires. .
On Fri, Sep 25, 2020 at 3:41 AM Jean-Paul Calderone < exarkun@twistedmatrix.com> wrote:
On Thu, Sep 24, 2020 at 8:54 PM Robert DiFalco <robert.difalco@gmail.com> wrote:
Why exactly is makeDirectory not firing any callbacks or errbacks and what can I add at the Factory level to make it do so?
class MySSHClientFactory(SSHClientFactory):
def clientConnectionLost(self, connector, reason): self.op.addErrback(reason)
def makeDirectory(self, path, attrs): self.op = Deferred() def _cbSuccess(result): self.op.callback(result)
return self.sftpClient.makeDirectory(path, attrs).addCallback(_cbSuccess)d
This code is wrong. It takes what *may* be a perfectly good Deferred from makeDirectory and breaks the error chain off of it. The correct way to do this would be more like `makeDirectory(...).chainDeferred(self.op)`. However, `self.op` and `_cbSuccess` are doing nothing useful here so you may as well just delete all that and return the `makeDirectory(...)` Deferred.
If *that* Deferred doesn't fire its errback chain when the connection is closed, that's a bug in Conch. If you can put together a minimal example and file a ticket, that would be wonderful and perhaps someone can get it fixed for you.
Jean-Paul _______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
participants (3)
-
Adi Roiban -
Jean-Paul Calderone -
Robert DiFalco