Opened 7 years ago

Last modified 7 years ago

#7840 enhancement new

allow ClientFactory objects to work properly in connect()

Reported by: TheAnarcat Owned by: TheAnarcat
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

although it is fairly well documented that the object passed to the endpoints.connect() functions is a Factory and not a ClientFactory, it is not explained why. furthermore, it makes it quite hard to react to lost and failed connexions elegantly. i *think* it could be done with the defer object, but it would require at least to go down in the protocol as well as in the factory as the connectionLost() even is simply not passed to the factory.

the attached patch fixes that: it will call those functions, if they exist (so it retains backward-compatibility). it my small tests here it works pretty well. i can, for example, reliably use ReconnectingClientFactory alongside clientFromString() without problems now.

sample use of this code is in the irklab irc client:

https://gitlab.com/anarcat/irklab/blob/master/irklab

Attachments (1)

7840-endpoints-factory.patch (2.4 KB) - added by TheAnarcat 7 years ago.

Download all attachments as: .zip

Change History (6)

Changed 7 years ago by TheAnarcat

comment:1 Changed 7 years ago by TheAnarcat

Keywords: review added

comment:2 Changed 7 years ago by Jean-Paul Calderone

Keywords: review removed
Milestone: Twisted 14.1.0
Owner: set to Glyph

Thanks for your interest in improving Twisted.

The decision not to call these methods was made intentionally. Perhaps it should be better explained - either in the documentation or in the implementation - so that it's clear it's not just an arbitrary limitation on functionality to make life more difficult.

Apart from that, unfortunately, the attached patch isn't acceptable for at least a couple of reasons:

  • It doesn't include any test coverage for the change being made. All changes to Twisted should be accompanied by changes to the automated test suite. However, this is moot in light of the next point...
  • There are many, many implementations of the endpoint interface. The attached patch changes the behavior of one of them but not all. Unfortunately, it's not possible to change all implementations now because not all implementations are even part of Twisted. The endpoint interface is partially intended as a point of extensibility to allow third-party libraries to supply new connection types to applications. Changing the behavior of the interface without changing all of those third-party implementations results in the problematic situation of having different endpoints provide different behavior so that you can never actually rely on the desired behavior being available.

If we were to agree that this is a desirable change to make in the behavior provided by endpoints, we would need to introduce a new interface (or, somehow, a new version of the interface) and then (optionally, but this part is pretty desirable) a compatibility layer between the two so that implementations of the old interface can still be used with code that desire the new behavior.

I'll leave this ticket open but I think it should be resolved by making the motivational documentation improvements I described at the top.

Also, I'm removing the milestone as the Twisted project uses milestones as part of the release planning process in a way that's unrelated to the topic of this ticket.

Again, thanks for your interest in making Twisted better.

comment:3 Changed 7 years ago by TheAnarcat

wait a minute here... the class that is patched is _WrappingFactory, which, according to the conventions that I know of, is a private class that is not supposed to be extended. in fact, it's pretty hard to override it which is the whole reason why i went with patching it like this instead of extending the existing code.

in other words, i don't see how that is a change in the interface *code* as much as in the interface /behavior/. and even there, the IStreamClientEndpoint class seems to explicitely that:

    A stream client endpoint is a place that L{ClientFactory} can connect to.
    For example, a remote TCP host/port pair would be a TCP client endpoint.

Ie. it says it expects a ClientFactory, and a ClientFactory has exactly those two functions on top of a regular Factory. The only place where this is documented otherwise is in the documentation file I patched. Any class that would be using that code in such a way would simply have had non-working code, ie. buggy code. It seems reasonable to fix what seems to me a bug in the interface.

Now, if you tell me there's another way to do this without patching twisted, I'm all ears. I'd also be happy to get more familiar with the unit-testing code and make unit tests if that's what is blocking review of this. However, if this is blocked at an earlier level, I am not going to spend much more time on this and try to find another way to make reconnexions work with defered objects...

That would be unfortunate, in my opinion.

comment:4 Changed 7 years ago by Jean-Paul Calderone

wait a minute here... the class that is patched is _WrappingFactory, which, according to the conventions that I know of, is a private class that is not supposed to be extended.

It's correct that _WrappingFactory is private. Code outside of Twisted is not meant to use it directly in any way.

The trouble is that what you want is for IStreamClientEndpoint.connect to provide this different behavior. If you implement the behavior in _WrappingFactory then the new behavior will be provided by all implementations of IStreamClientEndpoint.connect that use _WrappingFactory.

There's no guarantee that every IStreamClientEndpoint.connect implementation is going to use _WrappingFactory - and in fact, since it is private, no implementations outside of Twisted are even allowed to use it. Beyond that, not even all of the implementations inside of Twisted use it (for example, ProcessEndpoint does not use it).

So implementations of IStreamClientEndpoint.connect now have one of two possible behaviors. Either they call these methods (because they used _WrappingFactory) or they don't (because they don't). Application code trying to use IStreamClientEndpoint.connect has no way to know which behavior it will get. This somewhat defeats the purpose of having the interface and certainly makes life difficult for application developers.

and even there, the IStreamClientEndpoint class seems to explicitely that:

That's unfortunate. Someone did a bad job documenting (and then reviewing the documentation) for this interface. :/ This should instead say "factory" or "L{Factory}" or "L{IProtocolFactory}". It says "L{ClientFactory}" because of an editing mistake. But you already sort of knew it wasn't intended to completely work with ClientFactory because you found the other documentation that says it doesn't.

Any class that would be using that code in such a way would simply have had non-working code, ie. buggy code. It seems reasonable to fix what seems to me a bug in the interface.

Yes. Anyone who followed the incorrect documentation might have written code that didn't work. Hopefully they would have tested the code and found that it doesn't work and then done something else. If not, that's unfortunate and it's unfortunate that Twisted's incorrect documentation played a part in producing that outcome.

However, anyone who tested and fixed their code will not have such a bug - because the implementation does have some pretty reliable behavior. It never calls these methods. Anyone who tested their code would have easily discovered it doesn't behave how the interface documentation implies it behaves.

After the change represented by the attached patch, this is no longer the case. Some implementations (eg HostnameEndpoint) *will* call clientConnectionLost and clientConnectionFailed. Other implementations (eg ProcessEndpoint or, say, a third-party tor-based endpoint - which is a real example) will *not* call those methods. The result is that the interface has unreliable behavior and it's harder to draw any conclusions about whether a particular piece of code is buggy or not based on testing. That's an undesirable outcome.

Now, if you tell me there's another way to do this without patching twisted, I'm all ears.

There probably is. But the point isn't that we can't patch Twisted to improve this situation - it's just that the proposed patch probably isn't the way to do it.

Since it sounds like reconnection is your primary use-case, you might be interested in #4735.

Thanks again.

comment:5 Changed 7 years ago by Glyph

Owner: changed from Glyph to TheAnarcat
Note: See TracTickets for help on using tickets.