Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?
On Oct 1, 2013, at 1:13 AM, Laurens Van Houtven _...@lvh.io wrote: On Tue, Oct 1, 2013 at 12:59 AM, Glyph gl...@twistedmatrix.com wrote: Most of the code I can think of that wants to use that really wants the transport rather than the protocol, Yes, but having the protocol would also immediately give you access to the transport, and, from what I understand in most cases of AMP, also everything else :) The problem here is that you can do AMP decoding without a protocol anywhere in sight. A CommandLocator by itself, passed appropriate boxes to the callables returned by locateResponder, is capable of speaking AMP just fine, as long as you don't care about speaking it to an actual byte stream :-). (And ostensibly this is one way to speak AMP over transports other than BinaryBoxProtocol.) In the case of AMP routes (something it would be very nice to integrate into the main protocol), you have multiple command locators per transport, and each one might have its own properties that would be interesting to Argument types, This is why putting requirements on the thing-doing-the-parsing in the Argument definition makes sense; the Command could interrogate its prospective Responder class, asking each of its Argument objects if it will be able to satisfy their requirements in turn, at the time that CommandLocator.__metaclass__.__new__ gets invoked, rather than once your protocol is already trying to respond to commands. but nothing within AMP itself actually uses those arguments; in fact, searching the usual suspects (epsilon, vertex) I can't even find any Arguments that use the 'proto' argument for anything useful. I suppose it's too late to get proto to actually mean proto and not boxSender? I believe it currently means responderLocator. _wrapWithSerialization is where the magic happens if you want to confirm. It would definitely be a backwards-incompatible change, and I do actually have some code that somehow relied on it being the boxSender (actually, I think I saved that code in txampext, mostly, except I renamed that thing proto). It's probably too late for this method, but you can always add a new one :-) If I recall, I believe the idea behind it was to allow an AMP responder within Vertex to return the peer's IP address back to the peer, from within an authenticated AMP route that (because it was a route) wasn't necessarily connected directly to the transport (and therefore couldn't just do self.transport.getPeer()). Ironically I don't think it'll actually work for that now :-). When we pull the authentication logic in from http://bazaar.launchpad.net/~divmod-dev/divmod.org/trunk/view/head:/Epsilon/epsilon/ampauth.py, you might write a responder that's interested in authentication information that lives in some other relation to the protocol. I wrote very similar deep-in-AMP auth logic once, and did look at that code (but ended up not using it because I use TLS, so I don't need hand-rolled challenge response or OTP systems). The hand-rolled CR/OTP crap is really not supposed to be the interesting part. Really, that should just be SASL. The interesting part is the integration with cred. (Augh, and we didn't know the difference between a one-time password and a one-time pad, and the wrong word is right there in the wire format... Augh augh augh) When you say other relation to the protocol, does that mean it can be the protocol because the protocol will have some kind of reference to it? Notice that in that authentication code, CredReceiver *sets self.boxReceiver* to the result of portal.login. It hands off processing of the parsed AMP boxes to another IBoxReceiver. So the thing parsing the commands - the post-authentication protocol - is what the Arguments will currently have direct access to (as that will be the CommandLocator as well) but that object will have no transport; only a boxSender. So in order to fix fromBox/toBox, we need to do a fix that firms up that contract and perhaps exposes more than a Protocol object. The *recommended* API should be more or less like what ExposingArgument is doing - specify an Argument that asks for a particular attribute of the transport or the protocol or the authentication context or whatever, the implementation details may involve other lower-level public APIs. That still sounds like it can be done by making proto actually the proto ;-) So, basically, the question is if proto being the locator is a bug that I can fix, or an interface that I can't. You could add a new interface where it's fixed, but given the case I just described above, what does fixed mean? My contributions to AMP have been more of the defect-findy kind, but I could certainly turn them more into the code-contributy kind. I imagine I'm not the first person to want tests for command classes (https://github.com/lvh/txampext/blob/master/txampext/commandtests.py) or a
Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?
On Tue, Oct 1, 2013 at 12:59 AM, Glyph gl...@twistedmatrix.com wrote: Most of the code I can think of that wants to use that really wants the * transport* rather than the protocol, Yes, but having the protocol would also immediately give you access to the transport, and, from what I understand in most cases of AMP, also everything else :) but nothing within AMP itself actually uses those arguments; in fact, searching the usual suspects (epsilon, vertex) I can't even find any Arguments that use the 'proto' argument for anything useful. I suppose it's too late to get proto to actually mean proto and not boxSender? It would definitely be a backwards-incompatible change, and I do actually have some code that somehow relied on it being the boxSender (actually, I think I saved that code in txampext, mostly, except I renamed that thing proto). If I recall, I believe the idea behind it was to allow an AMP responder within Vertex to return the peer's IP address back to the peer, from within an authenticated AMP route that (because it was a route) wasn't necessarily connected directly to the transport (and therefore couldn't just do self.transport.getPeer()). Ironically I don't think it'll actually work for that now :-). When we pull the authentication logic in from http://bazaar.launchpad.net/~divmod-dev/divmod.org/trunk/view/head:/Epsilon/epsilon/ampauth.py, you might write a responder that's interested in authentication information that lives in some other relation to the protocol. I wrote very similar deep-in-AMP auth logic once, and did look at that code (but ended up not using it because I use TLS, so I don't need hand-rolled challenge response or OTP systems). When you say other relation to the protocol, does that mean it can be the protocol because the protocol will have some kind of reference to it? So in order to fix fromBox/toBox, we need to do a fix that firms up that contract and perhaps exposes more than a Protocol object. The *recommended* API should be more or less like what ExposingArgument is doing - specify an Argument that asks for a particular attribute of the transport or the protocol or the authentication context or whatever, the implementation details may involve other lower-level public APIs. That still sounds like it can be done by making proto actually the proto ;-) So, basically, the question is if proto being the locator is a bug that I can fix, or an interface that I can't. My contributions to AMP have been more of the defect-findy kind, but I could certainly turn them more into the code-contributy kind. I imagine I'm not the first person to want tests for command classes ( https://github.com/lvh/txampext/blob/master/txampext/commandtests.py) or a nested AMP box ( https://github.com/lvh/txampext/blob/master/txampext/nested.py). That would be cool. And, you know, that auth thing I said :-). If I can change proto to mean actually the protocol not something else then that seems plenty easy to add, and it would definitely be cool if people don't have to mess with this nonsense themselves for something as ostensibly simple as having access to the protocol :-) cheers lvh ___ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
[Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?
Hi everyone, I think I've hit one of those cases where AMP really seems to want everything (locator, receiver, sender) to be an instance of t.p.amp.AMP :-( I've written some code that tries to multiplex stream transports over AMP: https://github.com/lvh/txampext/blob/multiplexing/txampext/multiplexing.py The repo contains an example server and client, which demonstrate the issue: https://github.com/lvh/txampext/blob/multiplexing/docs/examples/multiplexing_client.py https://github.com/lvh/txampext/blob/multiplexing/docs/examples/multiplexing_server.py In order to do some of this multiplexing, I need access to the protocol instance inside the responder on the server side. Fortunately, I already had some code that exposed box senders (after a lot of advice from Glyph). I modified it to expose the protocol as well: https://github.com/lvh/txampext/blob/multiplexing/txampext/exposed.py#L41 However, it turns out fromBox gets called with the *responder locator* as the proto argument, not the actual protocol. The server has a pudb call that makes it easy (?!) to trace this down. The CommandLocator class, inside doit (a function defined in _wrapWithSerialization) passes self to command.parseArguments: https://twistedmatrix.com/trac/browser/trunk/twisted/protocols/amp.py#L1015 This is the part where I think the contract is broken, since parseArguments claims to want the protocol (well, it says it wants the AMP protocol, which, subclassing everything, is also all of the things, of course), but receives the responder locator. What am I doing wrong? Is this a bug? confusedly, lvh ___ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?
On Sep 30, 2013, at 2:45 AM, Laurens Van Houtven _...@lvh.io wrote: What am I doing wrong? Is this a bug? I think it's pretty clearly a bug. Calling the argument proto in the first place indicates the nature of the confusion. There are parts of the flow here from bytes to method execution and back (like _wrapWithSerialization) which are nice for composition, but the fact that they're private sort of ruins their utility for extensibility. Looking at the code you're trying to write in txampext though, the problem appears to be simply that you're writing functionality close enough to AMP's core that you should be making the changes to AMP directly, and fixing the issue by making changes to AMP itself rather than trying to work around it externally. The way I was going to recommend fixing it before I clicked on your link was by writing something like ExposingArgument and accessing the locator/receiver/sender via that new API rather than via the 'proto' argument at all :) -glyph___ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
Re: [Twisted-Python] AMP Argument.toBox's proto argument is a locator, not the proto?
On Sep 30, 2013, at 12:09 PM, Laurens Van Houtven _...@lvh.io wrote: Hi Glyph, Thanks for your response! On Mon, Sep 30, 2013 at 8:41 PM, Glyph gl...@twistedmatrix.com wrote: On Sep 30, 2013, at 2:45 AM, Laurens Van Houtven _...@lvh.io wrote: What am I doing wrong? Is this a bug? I think it's pretty clearly a bug. Calling the argument proto in the first place indicates the nature of the confusion. There are parts of the flow here from bytes to method execution and back (like _wrapWithSerialization) which are nice for composition, but the fact that they're private sort of ruins their utility for extensibility. Looking at the code you're trying to write in txampext though, the problem appears to be simply that you're writing functionality close enough to AMP's core that you should be making the changes to AMP directly, and fixing the issue by making changes to AMP itself rather than trying to work around it externally. The way I was going to recommend fixing it before I clicked on your link was by writing something like ExposingArgument and accessing the locator/receiver/sender via that new API rather than via the 'proto' argument at all :) I'm a little confused why that would help; you're saying there should be a new API that gives arguments access to the locator, receiver, sender? What would that look like? Something along the lines of fromBox/toBox, or are you thinking of a more direct approach where the locator has a reference to the other components? (Given your suggestion of not going through the proto argument, I imagine something closer to the latter.) For someone confused about why it would help, you are pretty close to the mark :). I am not trying to propose a specific new implementation mechanism, but rather to say that fromBox/toBox are broken, in that the contract of the 'proto' argument is incompletely specified. Most of the code I can think of that wants to use that really wants the transport rather than the protocol, but nothing within AMP itself actually uses those arguments; in fact, searching the usual suspects (epsilon, vertex) I can't even find any Arguments that use the 'proto' argument for anything useful. If I recall, I believe the idea behind it was to allow an AMP responder within Vertex to return the peer's IP address back to the peer, from within an authenticated AMP route that (because it was a route) wasn't necessarily connected directly to the transport (and therefore couldn't just do self.transport.getPeer()). Ironically I don't think it'll actually work for that now :-). When we pull the authentication logic in from http://bazaar.launchpad.net/~divmod-dev/divmod.org/trunk/view/head:/Epsilon/epsilon/ampauth.py, you might write a responder that's interested in authentication information that lives in some other relation to the protocol. So in order to fix fromBox/toBox, we need to do a fix that firms up that contract and perhaps exposes more than a Protocol object. The *recommended* API should be more or less like what ExposingArgument is doing - specify an Argument that asks for a particular attribute of the transport or the protocol or the authentication context or whatever, the implementation details may involve other lower-level public APIs. My contributions to AMP have been more of the defect-findy kind, but I could certainly turn them more into the code-contributy kind. I imagine I'm not the first person to want tests for command classes (https://github.com/lvh/txampext/blob/master/txampext/commandtests.py) or a nested AMP box (https://github.com/lvh/txampext/blob/master/txampext/nested.py). That would be cool. And, you know, that auth thing I said :-). I look forward to being in the same locality as you, I presume it will make me more productive ;) Living in that particular locale is going to spoil me. I feel like I may need to move somewhere more remote so that I am forced to have nice transparent discussions on the record like this one, on mailing lists on IRC :-). -glyph ___ Twisted-Python mailing list Twisted-Python@twistedmatrix.com http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
