AMP command-handling and protocol-parsing should be on separate objects

[glyph]

Just a specific case of the general rule that composition is better than inheritance.

This is a necessary prerequisite for #2529. In order for there to be such a thing as an "extension" to the protocol, there needs to be a way to separate out different pieces of the implementation, and a way to dispatch particular commands to different objects.

[hagna]

So, from this:

class AMP(StatefulStringProtocol, Int16StringReceiver,
          _AmpParserBase):

to this:

class AMP:

StatefulStringProtocol and Int16StringReceiver do protocol parsing and _AmpParserBase does command dispatching, right?

[exarkun]

This is more about what application code has to do in order to provide customized behavior relating to AMP. The factoring of the AMP implementation may also need to change, but that is a secondary concern.

So, for example, from this:

class MMORPG(AMP):
    ...

to this:

class MMORPG(object):
    ...
protocol = AMP(MMORPG())

(although this is really just an off-the-cuff example, I'm not sure this is ultimately the correct API to satisfy this ticket).

[glyph]

The existing APIs have to continue to be supported, of course. I like JP's example, but here's a brief sketch of the APIs that I personally expect from this ticket. These are not complete, but they should convey a general idea...

class IAMPMessageProcessor(Interface):
    """
    An L{IAMPMessageProcessor} provider can process messages for AMP.
    """
    def processMessage(box):
        """
        @param box: an AmpBox that contains a command, or a response to a
        previous command.
        """
    def ampConnectionMade():
        """
        ... maybe?  I don't know if these are necessary, they might be required for
        certain functionality.
        """
class MessageProcessorHelper(object):
    """
    Not the best name for this class, but this would implement the current
    default behavior of dispatching to messages; ideally with some
    nicely-documented, pluggable, internal APIs so that you could easily
    override areas of serialization or parsing without implementing everything
    yourself.  Many individual parts of these APIs are already defined within
    AMP, but not.

    Sort of like the existing _DispatchMixin, but public and supported.
    """
    implements(IAMPMessageProcessor)
class AMP(MessageProcessorHelper, ...):
    """
    The existing AMP protocol class.
    """
    def __init__(self, processor=None):
        """
        Create an AMP with the given message processor.
        """
        ...

    def processMessage(self, box):
        """
        Process a message.
        """
        if self.processor is not None:
            return self.processor.processMessage(box)
        else:
            return super(AMP, self).processMessage(box)
    def setMessageaProcessor(self, processor):
        """
        @param processor: an L{IAMPMessageProcessor} that will process messages
        sent to this transport.

        @raise ProtocolBusy: if the protocol has already been switched, then we
        are no longer handling messages and therefore have no message
        processor.
        """
        ...

The important thing is that these should pave the way for...

class MultiplexingMessageProcessor(object):
    """
    I look at a message and send it to one of several different potential
    objects.
    """
    implements(IAMPMessageProcessor)

    def __init__(self):
        self.processors = {}

    def processMessage(self, box):
        """
        Dispatch this box to an appropriate message processor based on the
        'processor' key.
        """
        return self.processors[box['processor']].processMessage(box)

In order to deal with protocol extensions, the AMP protocol itself would need to be upgraded to be a subclass of something similar to that.

Make sense?

[hagna]

Yes, that makes a lot more sense. Thanks JP and Glyph.

Would the method processMessage() from MessageProcessorHelper, whose name is not set in stone, do the same things as ampBoxReceived()?

[glyph]

Replying to hagna:

Yes, that makes a lot more sense. Thanks JP and Glyph.

No problem! I hope we can give you enough help to turn you into a regular Twisted contributor :).

Would the method processMessage() from MessageProcessorHelper, whose name is not set in stone, do the same things as ampBoxReceived()?

Yes. In fact ampBoxReceived is a better name anyway, so let's keep that.

[hagna]

class implementsMessageProcessor(amp.MessageProcessorHelper):
    pass 

def test_helloWorldCommandUsingCompose(self):
        """
        Verify that a simple command can be sent and its response received with
        the high-level value parsing API.
        """
        c, s, p = connectedServerAndClient(
            ServerClass=amp.AMP(implementsMessageProcessor).__class__,
            ClientClass=amp.AMP(implementsMessageProcessor).__class__)
        L = []
        HELLO = 'world'
        c.sendHello(HELLO).addCallback(L.append)
        p.flush()
        self.assertEquals(L[0]['hello'], HELLO)

Seems to take the right direction running through the new code, but

    def ampBoxReceived(self, box):
        if self.processor is not None:
            return self.processor.ampBoxReceived(box)  # new path
        else:
            return super(AMP, self).ampBoxReceived(box) # all the old tests do this
            # how many different tests should test the new path
            # before you know for sure that it works?

[hagna]

AMP can do AMP(mmorpg()) and the test case tries it

[hagna]

So what public pluggable API should MessageProcessorHelper implement?

The patch only includes enough to get what Glyph said working and one test case to show it. It doesn't raise the exception when it should though.

It includes a circular reference like AMP.processor.AMP.processor.AMP.processor.

This patch is better than nothing inasmuch as it generates feedback from you, please.

[Peaker]

Reattaching the same file with an extension known by trac

[Peaker]

I gave a short look.

• What is "class alpha"?
• Why all the extra whitespace?
• Is the code in MessageProcessorHelper.ampBoxReceived identical to the code below or have any changes been made?

[hagna]

class alpha and the extra whitespace are mistakes.

MessageProcessHelper.ampBoxReceived is the same as ampBoxReceived.

Did the test case work?

[glyph]

(In [21846]) Branching to 'compose-2810'

[glyph]

I am tempted to do all kinds of gold plating, adding interface declarations and a bunch more documentation, but I think this should go in for review now.

[glyph]

Feel free to suggest improvements for other tickets - and of course if I missed something important in this one :).

[therve]

OK let's go:

• ampBoxReceived really ought to be 3 submethods. This may not be the best place to do this, though
• I think the separation of CommandLocator and SimpleStringLocator doesn't worth it. Or maybe, CommandLocator should be a subclass of SimpleStringLocator.
• all the attributes of AMP should be declared at the top of the class (btw, the noPeerCertificate attribute is really lame).
• _ParserHelper still offers some methods as transport, althought it's not a AMP subclass anymore. and it's still used as a transport... Oh well that's weird.
• the expected interface of a boxSender is unclear. There's is this TODO in BoxDispatcher, this is a first step. But having _sendBox as a public method isn't natural to me.
• the fact that AMP is-a locator doesn't seem useful.
• there are some warnings to add, but you've noticed that.

Overall, I don't really know what to say. The previous implementation was complex, and this is as complex, but in a different way. The AMP class didn't loose parent classes, but in the contrary gain 2 more parents, whereas I thought one goal was to bring that down. Maybe this is an effect of backward compatibility, I don't know.

[glyph]

Replying to therve: OK let's go:

• ampBoxReceived really ought to be 3 submethods. This may not be the best place to do this, though
  • It's no problem to do this, and it would make the method shorter; but each of the methods would be a private method that was only called once.
• I think the separation of CommandLocator and SimpleStringLocator doesn't worth it. Or maybe, CommandLocator should be a subclass of SimpleStringLocator.
  • My goal was to very gradually move SimpleStringLocator to the sidelines. I don't think it was a particularly good idea and I left it there in AMP mainly for compatibility.
• all the attributes of AMP should be declared at the top of the class (btw, the noPeerCertificate attribute is really lame).
  • OK
• _ParserHelper still offers some methods as transport, althought it's not a AMP subclass anymore. and it's still used as a transport... Oh well that's weird.
  • This is a tiny helper utility. AMP still has a dependency on some transport stuff. The only real point to that change was to point out that, happily, that class no longer needs to inherit from AMP to do its job.
• the expected interface of a boxSender is unclear. There's is this TODO in BoxDispatcher, this is a first step. But having _sendBox as a public method isn't natural to me.
  • One of the things that I was thinking about doing was making actual Interface objects for each of the new interfaces. I thought this might be overkill, but maybe not; it would make it easier to describe the expected parameters everywhere. The way I plan to respond to this comment is to add them; I'm going to put them into the amp module, for lack of a better place to go. Obviously I will make _sendBox into sendBox in the process of doing so.
• the fact that AMP is-a locator doesn't seem useful.
  • This is entirely for compatibility. People are going to have AMP classes that define responders, and those need to keep working.
• there are some warnings to add, but you've noticed that.
  • Looking through the code the only mention I've made was in makeConnection, but you used a plural. Were there other warnings you'd like to suggest?

Overall, I don't really know what to say. The previous implementation was complex, and this is as complex, but in a different way. The AMP class didn't loose parent classes, but in the contrary gain 2 more parents, whereas I thought one goal was to bring that down. Maybe this is an effect of backward compatibility, I don't know.

This 90% an effect of backwards compatibility, and maybe 10% an effect of convenience (I do not intend to deprecate the old way of doing things, merely to suggest the new one: using just AMP for everything is messy, but valid, and easy to get started with). You will notice that AMP does not actually use any of the functionality of its superclasses; its implementation is much more independent now. Would you feel the implementation was less complex if there were a base AMP protocol with a subclass that mixed all these things together for convenience / compatibility? (Now that I put it that way it seems kind of obvious, I will do that.)

In summary: AMP is-a BoxDispatcher because previously code would have invoked callRemote on it. AMP is-a CommandLocator because previously code would have needed to define Command.responder methods on it. SimpleStringLocator is separated into a different class becuase it's really a separate idea and shouldn't be tied together with handling of Command objects.

[therve]

OK, great, this is more clear to me :). My comments below:

Replying to glyph:

Looking through the code the only mention I've made was in makeConnection, but you used a plural. Were there other warnings you'd like to suggest?

The other one I saw was CommandLocator.lookupFunction.

Would you feel the implementation was less complex if there were a base AMP protocol with a subclass that mixed all these things together for convenience / compatibility? (Now that I put it that way it seems kind of obvious, I will do that.)

Yep, I think that'd be more clear. Also, we'll be able to clearly define where we could make cleanups in the future.

[glyph]

I think I've dealt with all the feedback.

[therve]

First thing before I forgot:

these 2 objects' docstrings are not proper epytext:
    twisted.protocols.amp.IBoxSender.sendBox
    twisted.protocols.amp.BinaryBoxProtocol

• the implements declarations aren't tested.
• you've commented out the line self.recvd = '' in _switchTo. That probably means that something isn't tested, or that the previous comment is false.
• maybe some blank lines could be inserted around (after classes docstrings for example).

I think that's pretty cool, and I have only spotted some details, so if you're confident for merging, please do.

[glyph]

(In [22115]) re #2810 correct test to cover missing recvd line, put it back

[glyph]

(In [22116]) re #2810 valid epytext

[glyph]

(In [22117]) re #2810 more consistent use of whitespace after class docstrings

[glyph]

(In [22118]) re #2810 just to be sure

[glyph]

Given that I missed the uncovered line / potential regression in the last cycle, I'd like it if you gave it a final sign-off.

[therve]

• test_protocolSwitchLoseClientConnection has 'it shoult' in its docstring.

That's all I managed to find :). Please merge after the typo.

[glyph]

(In [22230]) Refactor AMP to separate sending and receiving binary boxes from handling them.

Author: glyph

Reviewer: therve

Fixes #2810

This change does not provide much in the way of new functionality, but it makes a group of previously opaque, private interfaces public and documented. In so doing, it creates a bunch of new, cleaner tests, and opens up a number of more obvious possibilities for implementing protocol-extension features such as streaming transfers, multiplexing (multiple command-handlers on a single socket), and authentication.