API documentation: self via Deferred

[pmhahn]

Please include the following information:

vncdotool version: v1.2.0 / git

We're using vncdotool to automate some GUI click-through testing. We added OCR to it running tesseract as some background process, which uses the Twisted reactor to fork and join these processes. Results are passed via Deferreds, which clashes with vncdotool's use of its Deferres, which must always return self.

1. On VNCDoToolFactory.clientConnectionMade the deferred is called with the instance of the protocol:

class VNCDoToolFactory(rfb.RFBFactory):
    protocol = VNCDoToolClient  # class
    def clientConnectionMade(self, protocol: VNCDoToolClient) -> None:
        self.deferred.callback(protocol)  # instance

2. Both vncdotool.command.build_command_list and vncdotool.api.ThreadedVNCClientProxy.getattr use code to get the unbound methods from the class VNCDoToolClient — not the bound method from the instance — which then require the 1st parameter self to be that reference to the instance.

class ThreadedVNCClientProxy:
    def __getattr__(self, attr: str) -> Any:
        method = getattr(self.factory.protocol, attr)  # from class
…
def build_command_list(…):
    client = VNCDoCLIClient  # class
…
            factory.deferred.addCallback(client.keyPress, key)  # from class

While this late binding of the instance allows command to build up the list of methods before the connection is even established, it is annoying in other cases where other information should be passed via the deferred and easily leads to errors, when a method does not return self at the end: then you get the dreaded None type has not attribute XXX error.

1. This needs to be documented somewhere.
2. maybe the internal implementation can be changed to use the bound methods.

PS: You also have to be very careful if your implementation raises any exception, which is translated to a Failure instance by Twisted and the deferred switches to errback mode, where only errbacks are executed until one returns a non-Failure instance. If you do not handle that case all following calls of vncdotool.api will be ignored until you reset the deferred to normal callback mode. Basically any raised Exception is fatal for vncdotool.api.

[jirutka]

I spent several hours digging in vnc-automate and vncdotool and trying to figure out why it doesn’t work in so obscure way and why are methods returning different types than what I see in the code… and then I found this issue.
Most of this comment has been self-censored.

[sibson]

The design goal of api.py is that is simulates a synchronous API, and shouldn't break users expectations.

This seems like we should change ThreadedVNCClientProxy to use self.protocol and reverse the build_command_list() and connect() calls in build_tool. We'd get syntax errors after connection which feels a little annoying but fixing that feels like it would be a larger change, that can come later.

If we can do something to reset the api to a working state for recoverable errors, then we should do so.