clientConnectionFailed() and other ClientFactory methods are not called for WAMP applications.

[mkohler]

Background

The old way to have a reactor call connect for you was to subclass twisted.internet.ClientFactory, create an instance of your class, and pass that to reactor.connectTCP.

Later versions of Twisted added the endpoints API, which provides a different way of having the reactor call connect. With the endpoints API, you pass twisted.internet.endpoints.clientFromString a string encoding the destinations (protocol, IP address, port) and you are returned a client object, which has a connect method. Then you call the connect method, passing it a transport factory.

According to the Twisted documentation, the endpoints API is preferred over ClientFactory for new code.

Autobahn, ClientFactory, and the Endpoints API

In general, the twisted/websocket examples use the ClientFactory/connectTCP API.

By contrast, the twisted/wamp examples use the endpoints API directly, or indirectly, by using autobahn.twisted.wamp.ApplicationRunner.

The Problem

autobahn.twisted.websocket.WebSocketClientFactory, which is used by WAMP applications, subclasses ClientFactory. However, since ApplicationRunner uses the endpoints API to make connections, the ClientFactory methods (startedConnecting, clientConnectionFailed, and clientConnectionLost) are never called, which is surprising and confusing.

According to the note "If you've used ClientFactory before..." on this page, it is the intended behavior of the endpoints API to not call ClientFactory methods.

Possible Solutions

• Have autobahn.twisted.websocket.WebSocketClientFactory subclass t.i.Factory instead of t.i.ClientFactory.

Given that the ClientFactory methods are not called for WAMP applications, it would be more clear if autobahn.twisted.websocket.WebSocketClientFactory subclassed twisted.internet.factory instead of twisted.internet.ClientFactory.

• Change ApplicationRunner to use ClientFactory instead of the endpoints API.

This would enable WAMP applications access to functionality that isn't
yet available with the endpoints API, e.g. https://twistedmatrix.com/trac/ticket/4735

[meejah]

Thanks for the nice summary!

My opinion is to subclass Factory and provide an Autobahn/WAMP-specific reconnect options. As the Twisted bug notes, there are two main cases: you never connected at all; or, you were connected fine but the connection went away. So there's sort of "retry" versus "re-connect" logic.

For WAMP there's conceptually two sub-cases on the "re-connect" side, too: if the client disconnected "on purpose" (e.g. called leave()) versus if the router (broker/dealer) side disconnected the client for some reason.

So what we want from this ticket are the building-blocks so that a Twisted user can express their desired retry and/or reconnect logic to Autobahn.

[oberstet]

@mkohler Thanks for the wrapup! We need to address this soon. AutobahnJS has a quite elaborated reconnection behavior - and we need that in AutobahnPython too.

In general, the twisted/websocket examples use the ClientFactory/connectTCP API.
By contrast, the twisted/wamp examples use the endpoints API directly, or indirectly, by using autobahn.twisted.wamp.ApplicationRunner.

Yep. This has mostly historic reasons. Essentially, all the examples should use endpoints.

Have autobahn.twisted.websocket.WebSocketClientFactory subclass t.i.Factory instead of t.i.ClientFactory

As you point out, it may be clearer to subclass t.i.Factory. Same for server classes.

In essence, I agree with @meejah : > My opinion is to subclass Factory and provide an Autobahn/WAMP-specific reconnect options.

For the options and the (re-)connect logic: see AutobahnJS https://github.com/tavendo/AutobahnJS/blob/master/package/lib/connection.js#L90

[oberstet]

related to #295

[oberstet]

As the tx base classes ClientFactory and ServerFactory don't add any functionality

• http://twistedmatrix.com/trac/browser/tags/releases/twisted-15.4.0/twisted/internet/protocol.py#L469
• http://twistedmatrix.com/trac/browser/tags/releases/twisted-15.4.0/twisted/internet/protocol.py#L141

I guess it's save when we just subclass Factory for both autobahn.twisted.websocket.WebSocketClientFactory and `autobahn.twisted.websocket.WebSocketServerFactory.

The reconnection thing is already worked on in different tickets.

[oberstet]

However, since ApplicationRunner uses the endpoints API to make connections, the ClientFactory methods (startedConnecting, clientConnectionFailed, and clientConnectionLost) are never called, which is surprising and confusing.

That's right, but the change I made has consequences #500

Essentially, it breaks connectTCP. Oh well.

[oberstet]

@mkohler So while it's confusing, subclassing ClientFactory works with both connectTCP and endpoints.

@meejah I'd say, backout the change. The only alternative I see is officially dropping support for using WebSocketClientFactory with connectTCP at all. Plus then changing most of our own WebSocket examples. Would that be better?

[oberstet]

Alright, I've backed out the change. I should have looked more closely. There is no issue anyway:

the ClientFactory methods (startedConnecting, clientConnectionFailed, and clientConnectionLost) are never called, which is surprising and confusing.

Correct. But the origin of this is Twisted going from old world (connectTCP) to new world (endpoints).

Hence: won't fix (here)

[meejah]

Since "the future is endpoints", yes that sounds like the right approach (changing the examples, but keeping connectTCP working for now). Perhaps if/when we change dependencies to Twisted 15+ we can drop not-endpoints...?