[Twisted-Python] IProtocolWithReactor, or passing the reactor though to protocols

[Amber "Hawkie" Brown]

(disclaimer: this is after several hours on an aeroplane, this may all be nonsense)

So, I'm currently looking into adding some features to twisted.web -- something that historically hasn't had the best support for pluggable reactors, and which requires levels and levels of monkeypatching (see the top level of https://github.com/twisted/twisted/compare/trunk...deferreds-in-resrender-3711 ). So, I was thinking, what if we made a new way for things to get their reactor, rather than extending all these concrete implementations everywhere?

Currently, you'd pass a reactor instance through to protocols by making the factory understand it, and the protocol calling self.factory._reactor or whatever. I never thought this was a supremely helpful or useful interface -- especially for one-shot classes -- so what if instead we made a new IProtocol-extending interface, which signals to tcp.Port/etc that the protocol, once created, should have a reactor set on it (maybe through some "setConnectingReactor" or something). This would mean that the factory doesn't know about the reactor in most cases (with things like HTTPFactory doing logging as an exception) -- but we could also have an IClient/ServerFactoryWithReactor, that Endpoints (which needs to know about the reactor, and which reactor it is listening on) can then check for and call a similar function, telling the Factory what reactor it is actually running under.

This, I think, would reduce a bunch of duplicate code in __init__s of factories, and could be implemented in the base class of Protocol or ServerFactory, possibly. What do people think?

- Amber

[Glyph Lefkowitz]

Post by Amber "Hawkie" Brown
(disclaimer: this is after several hours on an aeroplane, this may all be nonsense)
So, I'm currently looking into adding some features to twisted.web -- something that historically hasn't had the best support for pluggable reactors, and which requires levels and levels of monkeypatching (see the top level of https://github.com/twisted/twisted/compare/trunk...deferreds-in-resrender-3711 ). So, I was thinking, what if we made a new way for things to get their reactor, rather than extending all these concrete implementations everywhere?
Currently, you'd pass a reactor instance through to protocols by making the factory understand it, and the protocol calling self.factory._reactor or whatever. I never thought this was a supremely helpful or useful interface -- especially for one-shot classes -- so what if instead we made a new IProtocol-extending interface, which signals to tcp.Port/etc that the protocol, once created, should have a reactor set on it (maybe through some "setConnectingReactor" or something). This would mean that the factory doesn't know about the reactor in most cases (with things like HTTPFactory doing logging as an exception) -- but we could also have an IClient/ServerFactoryWithReactor, that Endpoints (which needs to know about the reactor, and which reactor it is listening on) can then check for and call a similar function, telling the Factory what reactor it is actually running under.
This, I think, would reduce a bunch of duplicate code in __init__s of factories, and could be implemented in the base class of Protocol or ServerFactory, possibly. What do people think?

First off, I think a clearer articulation of the problem would be helpful :). Is it "I don't want to take parameters in __init__"? Or "I don't want to have duplicated code to grab the global reactor everywhere, but I still want to grab the global reactor everywhere"? Is it "most factories don't need the reactor except to hand it to their protocols, therefore they shouldn't have to have code to deal with it at all"?

I definitely don't think we should address any of these issues with 'setConnectedReactor'. This is using a side-effect rather than just constructing the object with the things that it needs. We also shouldn't do it with a base class. Depending even more on inheritance would be movement in the wrong direction.

If we want to address the issue of duplicated code in constructors, how about something like a @reactorParameter(name='reactor') decorator, which does the grab-the-current-global-reactor-if-it's-not-passed thing? A better pattern is 'just pass in the reactor', of course.

Or, perhaps what you want is this 7-year-old ticket? :) https://twistedmatrix.com/trac/ticket/3205

-glyph

[Adi Roiban]

On 15 July 2016 at 22:06, Glyph Lefkowitz <***@twistedmatrix.com> wrote:
Or, perhaps what you want is this 7-year-old ticket? :)

Post by Glyph Lefkowitz
https://twistedmatrix.com/trac/ticket/3205

I left my feedback on the ticket :)

+1 for getting the reactor from the transport

[Amber "Hawkie" Brown]

Post by Glyph Lefkowitz

Post by Amber "Hawkie" Brown
(disclaimer: this is after several hours on an aeroplane, this may all be nonsense)
So, I'm currently looking into adding some features to twisted.web -- something that historically hasn't had the best support for pluggable reactors, and which requires levels and levels of monkeypatching (see the top level of https://github.com/twisted/twisted/compare/trunk...deferreds-in-resrender-3711 ). So, I was thinking, what if we made a new way for things to get their reactor, rather than extending all these concrete implementations everywhere?
Currently, you'd pass a reactor instance through to protocols by making the factory understand it, and the protocol calling self.factory._reactor or whatever. I never thought this was a supremely helpful or useful interface -- especially for one-shot classes -- so what if instead we made a new IProtocol-extending interface, which signals to tcp.Port/etc that the protocol, once created, should have a reactor set on it (maybe through some "setConnectingReactor" or something). This would mean that the factory doesn't know about the reactor in most cases (with things like HTTPFactory doing logging as an exception) -- but we could also have an IClient/ServerFactoryWithReactor, that Endpoints (which needs to know about the reactor, and which reactor it is listening on) can then check for and call a similar function, telling the Factory what reactor it is actually running under.
This, I think, would reduce a bunch of duplicate code in __init__s of factories, and could be implemented in the base class of Protocol or ServerFactory, possibly. What do people think?

First off, I think a clearer articulation of the problem would be helpful :). Is it "I don't want to take parameters in __init__"? Or "I don't want to have duplicated code to grab the global reactor everywhere, but I still want to grab the global reactor everywhere"? Is it "most factories don't need the reactor except to hand it to their protocols, therefore they shouldn't have to have code to deal with it at all"?
I definitely don't think we should address any of these issues with 'setConnectedReactor'. This is using a side-effect rather than just constructing the object with the things that it needs. We also shouldn't do it with a base class. Depending even more on inheritance would be movement in the wrong direction.
Or, perhaps what you want is this 7-year-old ticket? :) https://twistedmatrix.com/trac/ticket/3205
-glyph

Yes, that ticket is almost what I want! That is a much better solution for Protocols.

However, Factories still need the reactor given, and it seems really silly and very hard to explain why you both would need to create a Factory with a reference to a reactor, and then call reactor.listenTCP/endpoint.connect/listen, also with a reactor. I feel like one of these should really be able to tell the other.

This also doesn't fit the problem idnar raised on IRC -- that the reactor serving the connections may not be the one that the protocol wants to use, for whatever reason (gui reactor and a not gui reactor?) -- I can't say if #3205 has the same problems, but I believe it does. I guess then that's up to the protocol, but I kind of would like if all protocols had one consistent place for "this is the reactor I should schedule things on" -- and transport.reactor does not seem to be it, because that's still got the problem that idnar has raised.

- Amber

[Glyph Lefkowitz]

Post by Amber "Hawkie" Brown
Yes, that ticket is almost what I want! That is a much better solution for Protocols.
However, Factories still need the reactor given, and it seems really silly and very hard to explain why you both would need to create a Factory with a reference to a reactor, and then call reactor.listenTCP/endpoint.connect/listen, also with a reactor. I feel like one of these should really be able to tell the other.

Which factories need this reference given? Some need a clock, for e.g. timeouts, but then... that's OK, give them a clock. The fact that a parameter must sometimes be passed to two different places doesn't mean we shouldn't make it a parameter...

Possibly the problem here is that 'doStart' and 'doStop' ought to receive the reactor passed to them as well. And maybe something else, too, for that matter, like an address, or an endpoint - that interface has always been a bit annoyingly narrow. But a concrete use-case would help here.

Post by Amber "Hawkie" Brown
This also doesn't fit the problem idnar raised on IRC -- that the reactor serving the connections may not be the one that the protocol wants to use, for whatever reason (gui reactor and a not gui reactor?)

This use-case doesn't make any sense, because if you want to have a GUI you need to use a GUI reactor for everything; the reference passed is the running reactor.

Post by Amber "Hawkie" Brown
-- I can't say if #3205 has the same problems, but I believe it does. I guess then that's up to the protocol, but I kind of would like if all protocols had one consistent place for "this is the reactor I should schedule things on" -- and transport.reactor does not seem to be it, because that's still got the problem that idnar has raised.

Where did idnar raise this problem? I am still not clear on exactly what we're talking about.

-glyph

[Amber "Hawkie" Brown]

Post by Glyph Lefkowitz

Post by Amber "Hawkie" Brown
Yes, that ticket is almost what I want! That is a much better solution for Protocols.
However, Factories still need the reactor given, and it seems really silly and very hard to explain why you both would need to create a Factory with a reference to a reactor, and then call reactor.listenTCP/endpoint.connect/listen, also with a reactor. I feel like one of these should really be able to tell the other.

Which factories need this reference given? Some need a clock, for e.g. timeouts, but then... that's OK, give them a clock. The fact that a parameter must sometimes be passed to two different places doesn't mean we shouldn't make it a parameter...

Yes, but it's a very big footgun. If we are using a custom reactor, we must pass it at every level of the stack, when the information can easily be got from other sources.

Post by Glyph Lefkowitz
Possibly the problem here is that 'doStart' and 'doStop' ought to receive the reactor passed to them as well. And maybe something else, too, for that matter, like an address, or an endpoint - that interface has always been a bit annoyingly narrow. But a concrete use-case would help here.

My use case is making it easier to write things that require the reactor not having to be given absolutely everywhere -- it's very very inpenetrable to look at every interface and see if it has a reactor parameter.

Post by Glyph Lefkowitz

Post by Amber "Hawkie" Brown
This also doesn't fit the problem idnar raised on IRC -- that the reactor serving the connections may not be the one that the protocol wants to use, for whatever reason (gui reactor and a not gui reactor?)

This use-case doesn't make any sense, because if you want to have a GUI you need to use a GUI reactor for everything; the reference passed is the running reactor.

Different reactors in threads (e.g. gilectomy, STM).

Post by Glyph Lefkowitz

Post by Amber "Hawkie" Brown
-- I can't say if #3205 has the same problems, but I believe it does. I guess then that's up to the protocol, but I kind of would like if all protocols had one consistent place for "this is the reactor I should schedule things on" -- and transport.reactor does not seem to be it, because that's still got the problem that idnar has raised.

Where did idnar raise this problem? I am still not clear on exactly what we're talking about.

IRC, like I mentioned :)

Post by Glyph Lefkowitz
-glyph
_______________________________________________
Twisted-Python mailing list
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

[Glyph Lefkowitz]

Post by Amber "Hawkie" Brown

Post by Glyph Lefkowitz

Post by Amber "Hawkie" Brown
Yes, that ticket is almost what I want! That is a much better solution for Protocols.
However, Factories still need the reactor given, and it seems really silly and very hard to explain why you both would need to create a Factory with a reference to a reactor, and then call reactor.listenTCP/endpoint.connect/listen, also with a reactor. I feel like one of these should really be able to tell the other.

Which factories need this reference given? Some need a clock, for e.g. timeouts, but then... that's OK, give them a clock. The fact that a parameter must sometimes be passed to two different places doesn't mean we shouldn't make it a parameter...

Yes, but it's a very big footgun. If we are using a custom reactor, we must pass it at every level of the stack, when the information can easily be got from other sources.

How is passing a reactor parameter a 'footgun'? What is the misuse you anticipate that will cause people to habitually use incorrectly?

Post by Amber "Hawkie" Brown

Post by Glyph Lefkowitz
Possibly the problem here is that 'doStart' and 'doStop' ought to receive the reactor passed to them as well. And maybe something else, too, for that matter, like an address, or an endpoint - that interface has always been a bit annoyingly narrow. But a concrete use-case would help here.

My use case is making it easier to write things that require the reactor not having to be given absolutely everywhere -- it's very very inpenetrable to look at every interface and see if it has a reactor parameter.

You have to look at every interface to see what other parameters it has though. So I don't see how it is impenetrable. In fact it's required :).

Post by Amber "Hawkie" Brown

Post by Glyph Lefkowitz

Post by Amber "Hawkie" Brown
This also doesn't fit the problem idnar raised on IRC -- that the reactor serving the connections may not be the one that the protocol wants to use, for whatever reason (gui reactor and a not gui reactor?)

This use-case doesn't make any sense, because if you want to have a GUI you need to use a GUI reactor for everything; the reference passed is the running reactor.

Different reactors in threads (e.g. gilectomy, STM).

In that case we'd need an API for getting the reactor for the current thread (ideally when the thread was started). In that case it still wouldn't make much sense to pass a different one to a Protocol and a Factory.

Post by Amber "Hawkie" Brown

Post by Glyph Lefkowitz

Post by Amber "Hawkie" Brown
-- I can't say if #3205 has the same problems, but I believe it does. I guess then that's up to the protocol, but I kind of would like if all protocols had one consistent place for "this is the reactor I should schedule things on" -- and transport.reactor does not seem to be it, because that's still got the problem that idnar has raised.

Where did idnar raise this problem? I am still not clear on exactly what we're talking about.

IRC, like I mentioned :)

It would be good to get a write-up of the whole problem first, then. Making new functions that accept parameters is not a "problem" :).

-glyph

[Glyph Lefkowitz]

Post by Amber "Hawkie" Brown
(disclaimer: this is after several hours on an aeroplane, this may all be nonsense)
So, I'm currently looking into adding some features to twisted.web -- something that historically hasn't had the best support for pluggable reactors, and which requires levels and levels of monkeypatching (see the top level of https://github.com/twisted/twisted/compare/trunk...deferreds-in-resrender-3711 ). So, I was thinking, what if we made a new way for things to get their reactor, rather than extending all these concrete implementations everywhere?
Currently, you'd pass a reactor instance through to protocols by making the factory understand it, and the protocol calling self.factory._reactor or whatever. I never thought this was a supremely helpful or useful interface -- especially for one-shot classes -- so what if instead we made a new IProtocol-extending interface, which signals to tcp.Port/etc that the protocol, once created, should have a reactor set on it (maybe through some "setConnectingReactor" or something). This would mean that the factory doesn't know about the reactor in most cases (with things like HTTPFactory doing logging as an exception) -- but we could also have an IClient/ServerFactoryWithReactor, that Endpoints (which needs to know about the reactor, and which reactor it is listening on) can then check for and call a similar function, telling the Factory what reactor it is actually running under.
This, I think, would reduce a bunch of duplicate code in __init__s of factories, and could be implemented in the base class of Protocol or ServerFactory, possibly. What do people think?

First off, I think a clearer articulation of the problem would be helpful :). Is it "I don't want to take parameters in __init__"? Or "I don't want to have duplicated code to grab the global reactor everywhere, but I still want to grab the global reactor everywhere"? Is it "most factories don't need the reactor except to hand it to their protocols, therefore they shouldn't have to have code to deal with it at all"?

I definitely don't think we should address any of these issues with 'setConnectedReactor'. This is using a side-effect rather than just constructing the object with the things that it needs. We also shouldn't do it with a base class. Depending even more on inheritance would be movement in the wrong direction.

If we want to address the issue of duplicated code in constructors, how about something like a @reactorParameter(name='reactor') decorator, which does the grab-the-current-global-reactor-if-it's-not-passed thing? A better pattern is 'just pass in the reactor', of course.

Or, perhaps what you want is this 7-year-old ticket? :) https://twistedmatrix.com/trac/ticket/3205

-glyph