Implement something like ReconnectingClientFactory for endpoints

[glyph]

It's pretty easy to write a retry loop which will keep attempting an endpoint connection until it succeeds; this is mostly a documentation issue. However, another use-case for ReconnectingClientFactory is to keep a connection up after disconnections even if the initial connection succeeds just fine and only disconnects later; in other words, to react to clientConnectionLost vs. clientConnectionFailed. Implementing this would be easier if it were a little easier to cheaply dynamically proxy a protocol object to capture the connectionLost notification, but it's definitely possible to do this today with some care. We should provide such a utility so that people using the new endpoint APIs have a convenient mechanism available. (As we implement this, we should take care not to make the same naming and documentation mistakes that ReconnectingClientFactory does.)

[exarkun]

it's definitely possible to do this today with some care

Mind sketching out the API you have in mind?

[glyph]

Replying to exarkun:

it's definitely possible to do this today with some care

Mind sketching out the API you have in mind?

The thing I meant that it's "possible to do" is simply to write a wrapper for IProtocol which reliably captures connectionLost but doesn't actually get in the way of operation.

The top-level API I have in mind really just looks like this:

from twisted.application.internet import ClientKeeperUpper
ClientKeeperUpper(someClientEndpoint, someFactory[, reactor]).startService()

Possibly with some extra keyword arguments to control reconnection delays etc. stopService would loseConnection on the active connection, return a Deferred that fires when connectionLost gets called for that connection, and stop attempting to reconnect. Also, hopefully obviously, name subject to change.

[exarkun]

A couple considerations:

1. Services are great, but you don't always have an active application/service hierarchy when you want to do some reconnection. Not that this stops you from calling startService/stopService yourself.
2. There needs to be a way to reset the delay after an application-defined "success" event - the equivalent of "self.factory.resetDelay()" in a ReconnectingClientFactory-aware protocol.

[glyph]

Replying to exarkun:

A couple considerations: Services are great, but you don't always have an active application/service hierarchy when you want to do some reconnection. Not that this stops you from calling startService/stopService yourself.

Indeed not. And we should support these APIs a bit better. Maybe the reactor itself should implement IServiceCollection? That would be a neat way to propagate reactor parameters around, better than importing.

There needs to be a way to reset the delay after an application-defined "success" event - the equivalent of "self.factory.resetDelay()" in a ReconnectingClientFactory-aware protocol.

Yes. But, also, It seems like 'reset delay after successful connect' is a common enough use-case that it would be a good thing to be able to specify just as a boolean flag, so you don't need to write the callback hook yourself every time.

[jerub]

Current thinking is:

def nextDelay(now, last_success, last_failure):
    return delay_to_next_connection_attempt
pcs = PersistentClientService(endpoint, factory, reactor=reactor, nextDelay=nextDelay)
pcs.setServiceParent(rootservice)
pcs.connectedProtocol() # returns a deferred which will fire with a connected protocol.
client = MyClient(pcs) # MyClient may call pcs.connectedProtocol()
answer = client.do_something(argument) # call connectedProtocol.addCallback(lambda x:x.do_something(argument))

[dreid]

exarkun pointed out in an offline conversation that this probably isn't actually an IService I don't remember his reasons exactly but I can think of a couple.

1. You don't always have a service hierarchy. 2. You don't always have access to the service hierarchy when you know what you should connect to.

[glyph]

Replying to dreid:

exarkun pointed out in an offline conversation that this probably isn't actually an IService.

I definitely believe this should be an IService. It's a thing that can be started and stopped. There's no reason to invent a new, incompatible interface for that. (c.f. the  suffering caused by the fact that threadpools aren't IService providers.)

I don't remember his reasons exactly but I can think of a couple.

These are pretty crummy reasons.

1. You don't always have a service hierarchy.

You often do have a service hierarchy. If you don't, get one. If you can't get one, just call startService and stopService whenever you feel like starting or stopping your particular client connection.

2. You don't always have access to the service hierarchy when you know what you should connect to.

If you just want to hook up to a shutdown notification, there's always addSystemEventTrigger. But keeping a reference to the appropriate contact point in the appropriate service hierarchy is no more difficult than, say, passing a reference to the appropriate reactor to use.

[exarkun]

I'm somewhat convinced by the argument that it is a thing that can be started and stopped and that there is no reason to invent a new incompatible interface. However, then I can think of four reasons:

• IService.setName
• IService.setServiceParent
• IService.disownServiceParent
• IService.privilegedStartService

What do the implementations of these methods look like on the endpoint reconnecting IService implementation?

[glyph]

Replying to exarkun:

I'm somewhat convinced by the argument that it is a thing that can be started and stopped and that there is no reason to invent a new incompatible interface. However, then I can think of four reasons: * IService.setName * IService.setServiceParent * IService.disownServiceParent * IService.privilegedStartService What do the implementations of these methods look like on the endpoint reconnecting IService implementation?

Presumably these would be inherited from Service as is traditional for most IService implementors? I don't see why this one would be special.

[dreid]

(In [36948]) Branching to 'persistent-client-service-4735'

[acapnotic]

Okay, I wrote enough of this to be interesting, I think. Check it:  https://github.com/keturn/twisted/compare/trunk...persistent-client-service-4735

Do you have a better way to intercept that connectionLost message than my gross monkeypatching?

It seems like the caller might want to know when connection attempts and failures happen. Should the interface include any way to get notified of those? Or should we throw in some logging? Or can the caller pretty much get that information by customizing their Protocol and Factory methods? (Maybe you'd see connectionMade and connectionLost with that, but not ConnectionErrors?)

[exarkun]

Do you have a better way to intercept that connectionLost message than my gross monkeypatching?

With a wrapper - that way you have control over all events being sent to the protocol, since they're sent to your wrapper first.

It seems like the caller might want to know when connection attempts and failures happen.

The use case that seems most obvious to me is a way for the caller to know when a new connection has been established. This is not the connectedProtocol API, since that API only helps if the application uses it before each connection that is made.

Perhaps knowing when attempts are started and when failures happen is also important. This ticket is missing some use cases to help drive the specification.

Or can the caller pretty much get that information by customizing their Protocol and Factory methods?

They probably can, but this makes the API a lot less convenient.

[acapnotic]

Okay, using a wrapper:

 https://github.com/keturn/twisted/commit/615ae5eefc3f90ccc92ec168023605fcbe053c0a

I wrote that because I think that monkey-patching someone else's object would be terribly surprising and hard to debug, so I'd like to avoid that by any means necessary. However, I still find these means here to be pretty gross, as users may end up wondering "Hey, why did the Endpoint get passed this thing that's not actually my Factory? And why does it not actually return my Protocol?"

Trying to slip in a proxy on their Protocol like this still leaves me uneasy. How complete does the proxy have to be? Is it enough to expose only the things in the relevant Interface? I'd say no, given that protocol.transport and .factory are de-facto standard but not part of IProtocol. Which means it needs setattr as well. Do we need to be worried enough about attribute collision to do extreme things like use double-underscore?

A wrapper like this will catch calls to connectionLost from the outside, and the call from the Connection should be the one that counts, but it won't catch the protocol calling self.connectionLost on itself. (Which it really oughtn't do, but could be a surprising discrepancy.)

In summary, I think that if we throw in setattr on here it'll work in practice, but it doesn't leave me feeling great about saying "Twisted Endpoints are a great abstraction for all your internet needs!" (but no, I haven't come up with a better idea.)

[glyph]

Replying to acapnotic:

Okay, using a wrapper:  https://github.com/keturn/twisted/commit/615ae5eefc3f90ccc92ec168023605fcbe053c0a

Not to generalize prematurely, but we already have so many implementations of this (some within endpoints itself!) that it seems like it might be time to make it easier to create a protocol that you're delegating to.

I wrote that because I think that monkey-patching someone else's object would be terribly surprising and hard to debug, so I'd like to avoid that by any means necessary. However, I still find these means here to be pretty gross, as users may end up wondering "Hey, why did the Endpoint get passed this thing that's not actually my Factory? And why does it not actually return my Protocol?"

How would anyone get concerned about that? Isn't the whole point of this that the user of this API will leave calling connect up to the Service object? When you hand off a factory and endpoint to the PersistentClientService, you aren't allowed to have any particular expectations about what it will do with them beyond that it will do something to keep the protocol connected :).

The whole point of having a method like connectedProtocol is to allow it to expose the "right" object - which is not necessarily the result of endpoint.connect directly. You can un-wrap the wrapped result and return the wrapped protocol directly to clients of connectedProtocol.

Then nobody needs to see the wrapper - except perhaps in log messages. So maybe we need to bikeshed on the repr a little bit :).

Trying to slip in a proxy on their Protocol like this still leaves me uneasy. How complete does the proxy have to be?

It needs to be about as complete as the _WrappingProtocol that I linked to above. (Which means I think there ought to be some code sharing here.) There are other interfaces that the reactor might care about, related to junk like half-close, and we need to be careful to relay those reliably. But it doesn't need __setattr__.

A wrapper like this will catch calls to connectionLost from the outside, and the call from the Connection should be the one that counts, but it won't catch the protocol calling self.connectionLost on itself. (Which it really oughtn't do, but could be a surprising discrepancy.)

Do you have any example of code that actually does this for some reason? It's not clear to me what the expectations would be in that case, since if you called connectionLost on self, you'd already be having to deal with the duplicate notification from the transport when the connection was actually lost. If you tried to do something weird like taking the transport out of the reactor first, you'd still potentially leak a file descriptor or some other nasty side-effect.

[acapnotic]

When you hand off a factory and endpoint to the PersistentClientService, you aren't allowed to have any particular expectations about what it will do with them beyond that it will do something to keep the protocol connected :).

Ideally! But in my experience, when stuff breaks, you have cause to start tracing your call tree and your object graph, trying to figure out what's going on. When encountering things like the fact that dataReceived was not invoked on the Protocol you provided but on a _RestartableProtocolProxy, your wtfs-per-minute goes up and leads to statements like "that's why it's called Twisted."

More objects is more complexity, is all.

You can un-wrap the wrapped result and return the wrapped protocol directly to clients of connectedProtocol.

Oh, that's a good point, I hadn't thought of that. I do like the fact that would mean that the result of connectedProtocol is actually the object your ProtocolFactory returns. And that certainly reduces the need for proxying setattr.

(I was worried about the rather gauche practice Factory.buildProtocol indulges in, i.e. "protocol.factory = self", but since the user's factory does that to its own protocol, not the proxy, I think that's okay. And .transport is set through protocol.makeConnection, not as a direct attribute write.)

I can sort of imagine a situation somewhere where someone is doing introspection along the lines of filter(lambda connection: connection.protocol == myProtocol, connections), but I'm not actually sure if there's any such list of connections in a public API anywhere.

we already have so many implementations of this (some within endpoints itself!)

Yay code sharing! Also, apparently I get to stop worrying so much about whether this is a terrible thing to do, because it's already been done. (This does mean all these protocols will have wrappers from *both* the ClientEndpoint and the PersistentClientService, right?)

[glyph]

Replying to acapnotic:

When you hand off a factory and endpoint to the PersistentClientService, you aren't allowed to have any particular expectations about what it will do with them beyond that it will do something to keep the protocol connected :).

Ideally! But in my experience, when stuff breaks, you have cause to start tracing your call tree and your object graph, trying to figure out what's going on. When encountering things like the fact that dataReceived was not invoked on the Protocol you provided but on a _RestartableProtocolProxy, your wtfs-per-minute goes up and leads to statements like "that's why it's called Twisted."

Certainly we should keep an eye towards this, but "oh, there's another object in the middle" is a much less (ahem) "twisted" thing (in the pejorative sense) to do than to start introducing magic dynamical method-replacement. Keep in mind, also, that (at least for now) these stack traces are entirely straightforward and go from the reactor through the wrapper into their code; no roundabout call-traces where Deferreds cut the stack in half.

More objects is more complexity, is all.

Not necessarily. Having more explicit objects can often be less complexity than, for example, additional inheritance or monkey-patching or other forms of indirection. In this case we have a nice, explicit delegation, with every object simply adhering to the interfaces in the spots where it's expected.

You can un-wrap the wrapped result and return the wrapped protocol directly to clients of connectedProtocol.

Oh, that's a good point, I hadn't thought of that. I do like the fact that would mean that the result of connectedProtocol is actually the object your ProtocolFactory returns. And that certainly reduces the need for proxying setattr.

Reducing is good; eliminating would be better :). Are there remaining use-cases for proxying setattr that you can think of?

(I was worried about the rather gauche practice Factory.buildProtocol indulges in, i.e. "protocol.factory = self", but since the user's factory does that to its own protocol, not the proxy, I think that's okay. And .transport is set through protocol.makeConnection, not as a direct attribute write.)

Exactly. And the proxy doesn't need to use Factory.buildProtocol at all.

I can sort of imagine a situation somewhere where someone is doing introspection along the lines of filter(lambda connection: connection.protocol == myProtocol, connections), but I'm not actually sure if there's any such list of connections in a public API anywhere.

This is more an argument for propagating adaptation through the wrapper rather than getattr/setattr. If we had such an API, the right way to expresss its intention would be to say "I would like a list of every connected protocol that provides IFoo, because I'd like to do IFoo-ish things to it". For people who don't like zope interface, this could be expressed with an abstract base class instead of an interface; but the general idea is that you shouldn't call some random internal-guts-inspection API and expect to get objects that do what you want out of it; you need to declare what kind of expectations you have of the result so the innards of the API can sensibly filter and convert things for you.

If the API is for monitoring or metrics, then IProtocol is all you need; if it's for something more application specific, then you have to say which application. You can't have it both ways ;-).

we already have so many implementations of this (some within endpoints itself!)

Yay code sharing! Also, apparently I get to stop worrying so much about whether this is a terrible thing to do, because it's already been done. (This does mean all these protocols will have wrappers from *both* the ClientEndpoint and the PersistentClientService, right?)

That's true, and that's another reason that I really want to make sure we're calling into the same code here. We should (A) figure out a nice API to make this wrapping public, since other folks might want to do it, and (B) add some optimizations to collapse multiple wrappers into a single one, so you don't have to pay for stuff you're not using. In other words, in order to make this fast, it's likely that it will need to get a lot grosser, and I want to centralize and very thoroughly test any grossness that arises from a legitimate performance need, with benchmarks and everything; we shouldn't go straight to ad-hoc attribute setting because it "might be slow" to call through several layers of objects. Who knows: maybe PyPy's JIT will give the extra layers of method calls to us for free if they're fairly static and part of a loop, so it might even be a waste of time to add said grossness.

[dreid]

I'd propose that PersistentClientService should also accept a keyword argument whenConnected.

When whenConnected is not None it should be a single argument callable that will be called with the new protocol and that may return a deferred.

If it returns a deferred then subscribers should _not_ receive notification of the new protocol until that deferred fires.

Take for example the case of an IMAP4 backup utility.

It must: 1) connect 2) authenticate 3) select a mailbox 4) and then download messages in batches.

It would be easy to break down downloading messages in batches as.

• get connected protocol
• fetch N messages

Without a whenConnected notification and no public API to tell what the state of the protocol returned by connectedProtocol it is impossible to know if you have a previously connected protocol that is still viable, or a newly connected protocol that is in the incorrect state.

You could try your desired operation (such as fetch N messages) and handle failure as below:

* get connected protocol * try fetch N messages and handle failure by:

• getting connected protocol
• authenticate
• select mailbox
• try fetch N messages again

However this means you'd always have to try the operation at least once before you could do your initial setup, or maintain your own explicit state machine.

It seems cleaner to specify the set of operations that are done explicitly at new connection time prior to allowing connectedProtocol to return the protocol instance.

Another use case for whenConnected is something like IMAP4 IDLE and other server-push protocols (MQTT, AMQP, etc…) where (as exarkun pointed out) your application may never have the need or opportunity to call connectedProtocol.

[glyph]

Thanks for the thorough case-analysis, dreid.

[free.ekanayaka]

Can't the use cases mentioned by dreid be handled by implementing appropriate connectionLost() and connectionMade() on your protocol class, maybe with some state information on the factory?

[free.ekanayaka]

Can't the use cases mentioned by dreid be handled by implementing appropriate connectionLost() and connectionMade() on your protocol class, maybe with some state information on the factory?

[glyph]

Replying to free.ekanayaka:

Can't the use cases mentioned by dreid be handled by implementing appropriate connectionLost() and connectionMade() on your protocol class, maybe with some state information on the factory?

The problem is that it's a bit of a pain to stack and compose protocol objects, so the go-to way to override connectionLost on your own protocol is to simply inherit from the protocol you're trying to do something with. This is sub-optimal and it'd be great to discourage people from inheriting from protocols so much.

At least, I think that's what he's getting at; I'll let him speak for himself.

[free.ekanayaka]

Replying to glyph:

Replying to free.ekanayaka:

Can't the use cases mentioned by dreid be handled by implementing appropriate connectionLost() and connectionMade() on your protocol class, maybe with some state information on the factory?

The problem is that it's a bit of a pain to stack and compose protocol objects, so the go-to way to override connectionLost on your own protocol is to simply inherit from the protocol you're trying to do something with. This is sub-optimal and it'd be great to discourage people from inheriting from protocols so much. At least, I think that's what he's getting at; I'll let him speak for himself.

Yeah I can see the problem, and have gone through this myself a few times. It seems that most use cases are around the ability of being notified of connectionFailed() and connectionLost() (or IHalfCloseableProtocol events).

Of course adding a whenConnected parameter to PersistentClientService would solve some use cases in practice, but I'm curious if you think a more general solution would be worth. From what I've read, I gather that such solution would along the lines of #4700? (BTW the point that acapnotic makes in that ticket about pub/sub vs stacking/composition is interesting).

[dreid]

Replying to free.ekanayaka:

Replying to glyph:

Replying to free.ekanayaka:

Can't the use cases mentioned by dreid be handled by implementing appropriate connectionLost() and connectionMade() on your protocol class, maybe with some state information on the factory?

The problem is that it's a bit of a pain to stack and compose protocol objects, so the go-to way to override connectionLost on your own protocol is to simply inherit from the protocol you're trying to do something with. This is sub-optimal and it'd be great to discourage people from inheriting from protocols so much. At least, I think that's what he's getting at; I'll let him speak for himself.

Yeah I can see the problem, and have gone through this myself a few times. It seems that most use cases are around the ability of being notified of connectionFailed() and connectionLost() (or IHalfCloseableProtocol events). Of course adding a whenConnected parameter to PersistentClientService would solve some use cases in practice, but I'm curious if you think a more general solution would be worth. From what I've read, I gather that such solution would along the lines of #4700? (BTW the point that acapnotic makes in that ticket about pub/sub vs stacking/composition is interesting).

The key feature of whenConnected (and something which is difficult to achieve with connectionMade) is that it may return a Deferred and in doing so "block" access to the connected protocol resource until it has finished doing all of the necessary setup.

[free.ekanayaka]

Thanks for explaining. I had missed the relationship between the deferred returned by whenConnected() and the consequent "blocking" behavior with respect to connectedProtocol(). I can know see how it covers very well the typical use cases (including server-push protocols, as pointed).