Unfuck factories

[gdude2002]

Here's the current path we take when we connect a protocol.

• FactoryManager instanciates Factory with the name of the protocol, its config and the factory manager instance.
• FactoryManager calls Factory.setup() to begin the process.
• The Factory loads up the corresponding protocol module if it hasn't been loaded already - or reloads it if it has been.
• Factory then instanciates the protocol with its name, config, and the instance of the Factory.
• The protocol, in its __init__, calls reactor.connect<proto>(...), where <proto> is the connection protocol, passing in the instance of the factory.
• To create the connection, Twisted calls Factory.buildProtocol(addr), which returns the current instance of the protocol that we've already created.

Twisted, however, expects something like this:

• The Factory is instanciated.
• reactor.connect<proto>(...) is called, passing in the instance of the factory.
• Twisted calls Factory.buildProtocol(addr), which creates an instance of the protocol and returns it, allowing Twisted to connect with it.

What would be our ideal path? Let's fix that and make it happen! As a side effect, this ticket will result in protocols having their own factories, which is a requirement for all kinds of things.

[gdude2002]

In my opinion, I think we should do something like..

• FactoryManager instanciates the Factory as before and calls setup().
• Factory is a protocol-specific class derived from a base class that does the reconnection things in a Twisted way.
  • buildProtocol would indeed build a protocol
  • We need a connect() function as well, so we can generify without losing flexibility
  • This would be in charge of calling reactor.connect<proto>().
  • setup() does all of the connection-specific stuff, but doesn't instanciate the protocol and all it should do is track it
  • setup() should call connect() to get things started.
• To avoid breaking the API, we should keep the protocol attribute as-is, but I don't think that should be a problem.

[rakiru]

What would setup() do exactly? Isn't everything covered by connect() and buildProtocol()?

[gdude2002]

Ah, hm, yeah, I suppose that's all one would need.. I can't think of anything I'd put in setup() that would be bad to run more than once.

[rakiru]

Why would we run more than once? Isn't the point that connect/setup() gets called once to start it, and buildProtocol() gets called on each connection? What needs done other than building the protocol? Reloading it, but that's currently done at protocol build time anyway.

[gdude2002]

It would be run again if the connection fails or is lost, of course. Not all factories would be calling the same reactor.connect<proto>(...) function with the same args, and we need some generic way of doing this if we want to have reconnect management in the base protocol.

[rakiru]

When would connect() be called other than to begin with though? ClientFactory already has a way to deal with this. Considering ReconnectingClientFactory. You only call reactor.connect... on it once, then it uses the connector to schedule a reconnect when clientConnectionLost/Failed is called.

[gdude2002]

OK, sure, but then the protocol instance gets reused - do we want that?

[rakiru]

What? It calls buildProtocol()...

[gdude2002]

Oh, it does? I didn't know that. Alright then, then... yeah. That.

[gdude2002]

Time for another draft, I guess.

• FactoryManager instanciates the Factory as before and calls connect().
• Factory is a protocol-specific class derived from a base class that does the reconnection things in a Twisted way.
  • buildProtocol() would indeed build a protocol, and keep track of the latest protocol instance
  • We need a connect() function, so we can generify without losing flexibility
  • This would be in charge of getting everything loaded and calling reactor.connect<proto>()
  • When we handle reconnections, we should call connector.connect() instead of rebuilding everything by calling self.connect()
  • Perhaps we should consider staggering our reconnections by delaying them exponentially like so many other similar services (bitlbee)
• To avoid breaking the API, we should keep the protocol attribute as-is, but I don't think that should be a problem.

[rakiru]

What's the point of setup()? Isn't that the point of __init__()? Better reconnect logic (re. timing) would be a good idea. If we need to keep track of the Protocol instance anyway, no point breaking the API by renaming protocol attribute.

[gdude2002]

Forgot to rename to connect(). :P

[gdude2002]

Progress on this is progressing nicely. Couldn't think of a better way to put that.

@rakiru I think you're good to review the commits so far.

[gdude2002]

We're done - #91