Implement AMP version 2 #1417

jothan · 2020-10-02T14:30:27Z

Implement AMP v2 as described here: https://amp-protocol.net/conversations_v2.html

Contributor Checklist:

The associated tickets in Trac are here:
https://twistedmatrix.com/trac/ticket/2529
https://twistedmatrix.com/trac/ticket/9488
I ran tox -e black-reformat to format my patch to meet the Twisted Coding Standard
I ran additional checks listed at Getting Your Patch Accepted
I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
I have updated the automated tests.
I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.

rodrigc

Thanks for the contribution

src/twisted/protocols/amp.py

src/twisted/test/test_amp.py

jothan · 2020-10-02T17:30:18Z

@rodrigc I have added the type annotations as you requested. May I resolve the comments you added ?

glyph · 2020-10-02T17:36:46Z

src/twisted/protocols/amp.py

@@ -2613,6 +2634,35 @@ def connectionLost(self, reason):
        self.transport = None


+class AMPv2(AMP):


This class and all added methods here need docstrings.

rodrigc · 2020-10-02T17:58:32Z

@jothan At the top-level Twisted directory, you can run tox -e mypy to check things before mypy before pushing to GitHub. That will save a few cycles.

jothan · 2020-10-02T18:07:10Z

@rodrigc Thanks for the tip. However, I'm a bit stumped by the remaining errors, do you have any tips ?

jothan · 2020-10-02T19:01:37Z

Alright, I figured out which type annotations I needed, mypy is now happy.

src/twisted/protocols/amp.py

rodrigc · 2020-10-02T19:02:45Z

Thanks for being patient and submitting the annotations. This is slowly making the Twisted codebase better, and easier to understand.

jothan · 2020-10-02T19:08:04Z

@rodrigc I totally understand the need for those annotations, especially in a large code base.

My AMP implementation is in Rust with types everywhere ;P https://github.com/jothan/amp-async

rodrigc

I think this looks OK, but I'll leave open for another reviewer to give it a once over.

Nice to see you have experience writing AMP protocol parsers in Rust. I need to learn Rust one of these days. :)

glyph

I'm sorry that I didn't have time to get in a code review before this landed, but my attention was brought to it and I have some post-hoc feedback.

glyph · 2020-10-19T17:00:52Z

src/twisted/newsfragments/9488.feature

@@ -0,0 +1 @@
+Implement the AMP version 2 protocol to allow values greater than 64k.


We shouldn't have duplicate NEWS entries like this, as it produces useless duplicative noise in the changelog. The correct thing to do here is to close the second ticket as a duplicate in trac, then have there be one NEWS file.

You might want to update the guidelines in this case, from the news fragment instructions: " You should replace with the ticket number which is being resolved by the change (if multiple tickets are resolved, multiple files with the same contents should be added). "

@jothan Hmm. Apparently I'm wrong about this. I thought that this was an idiom that predated our usage of towncrier, but apparently towncrier adopted this same idiom of hashing the contents of the files; I just created a sample project to test it out and it does the right thing in all the edge cases that I thought would not be handled. Sorry for the mistake! I am still probably going to be rolling this back out for the other reasons discussed below, but at least we can forget about this...

glyph · 2020-10-19T17:01:05Z

src/twisted/protocols/amp.py

@@ -239,6 +239,7 @@ def trapZero(result):

 __all__ = [
    "AMP",
+    "AMPv2",


This should not be exposed as a separate class, but rather a protocol-version parameter.

glyph · 2020-10-19T17:01:26Z

src/twisted/protocols/amp.py

@@ -608,7 +609,7 @@ class IncompatibleVersions(AmpError):
 PROTOCOL_ERRORS = {UNHANDLED_ERROR_CODE: UnhandledCommand}


-class AmpBox(dict):
+class AmpBox(Dict[bytes, bytes]):


We really should have landed type annotations as a separate, prior change. They're not related to "AMPv2".

I know, sorry for the noise, I wanted to follow trunk closely with tests passing.

glyph · 2020-10-19T17:01:51Z

src/twisted/protocols/amp.py

@@ -657,15 +658,15 @@ def copy(self):
        newBox.update(self)
        return newBox

-    def serialize(self):
+    def serialize(self, version2: bool = False) -> bytes:


This "version2" parameter needs a @param in the docstring.

glyph · 2020-10-19T17:02:20Z

src/twisted/protocols/amp.py

+                vio = BytesIO(v)
+                del v
+
+                # If the value is an exact multiple of 65535, the last


how is the value "an exact multiple" of 65535 if it is not 65535 itself?

Guess I should remove the word "exact" here.

glyph · 2020-10-19T17:03:22Z

src/twisted/protocols/amp.py

+                # (i.e. when read() returns EOF).
+                chunk = vio.read(MAX_VALUE_LENGTH)
+                while True:
+                    w(pack("!H", len(chunk)))


I realize that this is coming from the "spec" on amp-protocol.net, but there were never any discussions I saw about how to implement a V2. Given that this is neither backwards compatible nor has any wire-level negotiation it seems like a pretty bad feature for a version bump.

(A better way to have done this would be to normalize the meaning of repeated keys, but I digress.)

Well, it is mostly backwards compatible as long as values are smaller than 65534 bytes. Considering it is a breaking change, it should be signaled via a compatibility version change. I would have called AMP v1.1, but AMP v2 already existed conceptually.

If you have a key which is just a bit over 64k bytes, you end up screwing up the alternation of keys and values, which means that the failure mode involves the peer hanging forever; that's a pretty bad failure mode.

There's a way to do this which is entirely backwards compatible, though; just send the same key repeatedly, or send a key with .1, .2, etc appended to it. This ends up adding 0.02% or so worth of overhead to the data (and if you have really large data, you want what's described on https://twistedmatrix.com/trac/ticket/2529 anyway, not just a way to circumvent the length-prefix limit).

You can also do this as a new parameter-type declaration, which also allows you to interrogate your peer's capabilities by checking for an UNHANDLED response if you have something you want to upgrade.

If we wanted to do something dramatically better, netstrings are the way to go; arguably AMP should have used these in the first place.

glyph · 2020-10-19T17:04:46Z

src/twisted/protocols/amp.py

-    def callRemoteString(self, command, requiresAnswer=True, **kw):
+    def callRemoteString(
+        self, command: bytes, requiresAnswer: bool = True, **kw: bytes
+    ) -> Optional[Deferred]:


Optional[Deferred] is the wrong way to specify this signature; what you want is an @overload to correlate the value of requiresAnswer to the type of the response.

glyph · 2020-10-19T17:06:36Z

src/twisted/protocols/amp.py

+        assert self.transport is not None
+        self.transport.write(box.serialize(version2=True))
+
+    def proto_value(self, string: bytes) -> str:


These methods do not belong on an AMP subclass, if they belong on a subclass at all, but a BinaryBoxProtocol subclass.

There's a mea-culpa here too; the inheritance-for-convenience design of AMP was a mistake, and should not have created this dichotomy; but, this is also why this should not be a new class, though, and instead just a parameter to BinaryBoxProtocol that AMP can propagate.

Yeah, I got hit by that. My initial unpublished patch was version parameters everywhere.

glyph · 2020-10-20T17:37:09Z

@jothan in case you are not on the Twisted mailing list, I began a discussion about this here: https://twistedmatrix.com/pipermail/twisted-python/2020-October/065288.html

I'm leaning towards doing a revert based on that.

Thanks for your contribution to Twisted, sorry to burden it with more work!

glyph · 2020-10-21T04:56:13Z

@jothan Would you mind re-submitting just the type-annotation bits of this PR first, since they're not really related to the other stuff happening here, and would be useful to integrate on their own?

jothan added 2 commits October 2, 2020 10:14

Send side AMPv2.

e92629c

Receive side AMP v2.

fc08303

jothan changed the title ~~2529 jothan ampv2~~ Implement AMP version 2 Oct 2, 2020

jothan added 2 commits October 2, 2020 10:34

NEWS entries.

3e4526f

Test for long values with AMPv2.

bd95f5c

jothan marked this pull request as ready for review October 2, 2020 15:56

rodrigc suggested changes Oct 2, 2020

View reviewed changes

Add type annotations.

0bf1db7

Also annotate return types.

6993c2d

glyph reviewed Oct 2, 2020

View reviewed changes

jothan added 2 commits October 2, 2020 13:46

Docstrings

765fba7

Try to make mypy happy.

2902f27

Write more Rust in Python.

3b9cb62

jothan added 2 commits October 2, 2020 14:31

More annotations, some asserts.

c211f0d

Finally satisfy mypy.

b24d83c

jothan requested a review from rodrigc October 2, 2020 19:01

rodrigc suggested changes Oct 2, 2020

View reviewed changes

src/twisted/protocols/amp.py Outdated Show resolved Hide resolved

Be nice to Python 3.5 for now.

9ce8f89

jothan requested a review from rodrigc October 2, 2020 19:34

jothan added 2 commits October 2, 2020 17:43

Merge branch 'trunk' into 2529-jothan-ampv2

e6e0df6

Merge branch 'trunk' into 2529-jothan-ampv2

8838308

rodrigc approved these changes Oct 3, 2020

View reviewed changes

jothan requested a review from glyph October 4, 2020 00:26

Merge branch 'trunk' into 2529-jothan-ampv2

656be86

jothan and others added 9 commits October 3, 2020 21:11

Merge branch 'trunk' into 2529-jothan-ampv2

5cd099d

Merge branch 'trunk' into 2529-jothan-ampv2

1e7ee49

Merge branch 'trunk' into 2529-jothan-ampv2

4799ef0

Merge branch 'trunk' into 2529-jothan-ampv2

94e8ce1

More mypy.

85667ac

Fix unreachable statement according to mypy.

08eacf7

Merge branch 'trunk' into 2529-jothan-ampv2

ff85fbf

Merge branch 'trunk' into 2529-jothan-ampv2

6f3f8c2

Useless type annotation.

b90242f

rodrigc merged commit f6980e3 into twisted:trunk Oct 19, 2020

glyph reviewed Oct 19, 2020

View reviewed changes

glyph mentioned this pull request Oct 21, 2020

Revert "Implement AMP version 2" #1455

Merged

jothan mentioned this pull request Oct 21, 2020

Add type annotations to AMP. #1459

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement AMP version 2 #1417

Implement AMP version 2 #1417

jothan commented Oct 2, 2020 •

edited

Loading

rodrigc left a comment

jothan commented Oct 2, 2020

glyph Oct 2, 2020

rodrigc commented Oct 2, 2020

jothan commented Oct 2, 2020

jothan commented Oct 2, 2020

rodrigc commented Oct 2, 2020

jothan commented Oct 2, 2020 •

edited

Loading

rodrigc left a comment

glyph left a comment

glyph Oct 19, 2020

jothan Oct 19, 2020

glyph Oct 21, 2020

glyph Oct 19, 2020

glyph Oct 19, 2020

jothan Oct 19, 2020

glyph Oct 19, 2020

glyph Oct 19, 2020

jothan Oct 19, 2020

glyph Oct 19, 2020

glyph Oct 19, 2020

jothan Oct 19, 2020

glyph Oct 21, 2020

glyph Oct 19, 2020

glyph Oct 19, 2020

jothan Oct 19, 2020

glyph commented Oct 20, 2020

glyph commented Oct 21, 2020

		@@ -2613,6 +2634,35 @@ def connectionLost(self, reason):
		self.transport = None


		class AMPv2(AMP):

		@@ -0,0 +1 @@
		Implement the AMP version 2 protocol to allow values greater than 64k.

Implement AMP version 2 #1417

Implement AMP version 2 #1417

Conversation

jothan commented Oct 2, 2020 • edited Loading

Contributor Checklist:

rodrigc left a comment

Choose a reason for hiding this comment

jothan commented Oct 2, 2020

Choose a reason for hiding this comment

rodrigc commented Oct 2, 2020

jothan commented Oct 2, 2020

jothan commented Oct 2, 2020

rodrigc commented Oct 2, 2020

jothan commented Oct 2, 2020 • edited Loading

rodrigc left a comment

Choose a reason for hiding this comment

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph commented Oct 20, 2020

glyph commented Oct 21, 2020

jothan commented Oct 2, 2020 •

edited

Loading

jothan commented Oct 2, 2020 •

edited

Loading