Implement multiplexer #835

pipermerriam · 2019-07-24T18:00:39Z

Extracted from #684

depends on #836

depends on #838

should be merged after #834

What was wrong?

In order to facilitate speaking multiple devp2p protocols, we need mechanisms for dividing the messages up by their individual protocol streams.

How was it fixed?

This is part of #684 It brings in a single component, the p2p.multiplexer.Multiplexer. This class is intentionally not a service. This is because it must be used outside of the service context during the handshake process to facilitate individual protocol handshakes streaming only the messages pertinent to that protocol.

This PR only introduces the class and the relevant tests for it in order to keep the PR adequately small.

You can see how this API is used in this commit: c6fb7d8

To-Do

Clean up commit history

Add entry to the release notes

Cute Animal Picture

pipermerriam · 2019-07-24T20:16:03Z

@carver maybe hold on the review until the dependent PR's are merged and I can rebase this.

carver · 2019-07-26T00:39:49Z

@carver maybe hold on the review until the dependent PR's are merged and I can rebase this.

Ok, sounds good. I was 😨 looking at the +/-

pipermerriam · 2019-07-26T02:19:43Z

@carver I have one more piece I'll split out tomorrow but if you want to look at p2p.multiplexer.Multiplexer and the corresponding tests in tests/p2p/test_multiplexer.py those are not going to change.

pipermerriam · 2019-07-26T14:46:35Z

@carver #850 and then I can rebase down to my intended review target.

pipermerriam · 2019-07-27T02:31:47Z

Alright @carver . This is as small as it is getting without doing something like committing the p2p.abc.MultiplexerAPI class on it's own or delaying the MultiplexerPairFactory to come after this PR, neither of which seem right to me.

carver · 2019-07-30T00:11:40Z

Okay, great. I am on it. Sorry that I didn't get it done today. I promise, first thing tomorrow.

carver

I'm going to try something different: I essentially only read the tests, and commented on the API along the way. I said some things that will sound dumb after I read the implementation, I'm sure. :)

But I thought it was a nice way to give a first-impression of the API, in the last moments of being ignorant of how it actually works under the hood... Okay, on to the implementation.

carver · 2019-07-30T17:59:32Z

tests/p2p/test_multiplexer.py

+@pytest.mark.asyncio
+async def test_multiplexer_properties():
+    multiplexer, _ = MultiplexerPairFactory(
+        protocol_types=(SecondProtocol, ThirdProtocol),


Since we're dropping the second mux here, I was curious what it looks like to build a single Mux. Is this right, below? It could be nice to have this either in the test, or maybe in its own API, like Multiplexer.build().

def build(transport, subprotocol_types, base_protocol_type=P2PProtocol, snappy_support=True, cancel_token=None): noop_transport = NoopTransport() p2p_protocol = P2PProtocol(noop_transport, snappy_support=snappy_support) subprotocol_types = (SecondProtocol, ThirdProtocol) cmd_id_offsets = get_cmd_offsets(subprotocol_types) sub_protocols = tuple( protocol_class(noop_transport, offset, snappy_support=snappy_support) for protocol_class, offset in zip(subprotocol_types, cmd_id_offsets) ) return Multiplexer( transport=noop_transport, base_protocol=p2p_protocol, protocols=sub_protocols, )

carver · 2019-07-30T18:09:26Z

tests/p2p/test_multiplexer.py

+            bob_stream = bob_multiplexer.stream_protocol_messages(P2PProtocol)
+
+            alice_p2p_protocol = alice_multiplexer.get_protocol_by_name('p2p')
+            bob_p2p_protocol = bob_multiplexer.get_protocol_by_name('p2p')


Is there a reason to favor by_name over by_type? The by_type is nicer to constrain at type-checking time.

And if we do prefer by_type, how bad would it be to drop the by_name option altogether?

I'll check the code that's using this in the bigger branch and see if I can remove the by_name APIs cleanly.

carver · 2019-07-30T18:12:18Z

tests/p2p/test_multiplexer.py

+            bob_p2p_protocol = bob_multiplexer.get_protocol_by_name('p2p')
+
+            alice_p2p_protocol.send_ping()
+            cmd, _ = await asyncio.wait_for(bob_stream.asend(None), timeout=0.1)


.asend(None) feels awkward, like it requires you to know something about the internals of how the stream works. Maybe it makes sense to add an alias, so you can do something likebob_stream.get()?

These are just vanilla python async generators and I'm calling their internal APIs for getting the next value from them where normally you would use these as async for thing in bob_stream.

I can look into another way to structure the tests but this was the most direct way I could think of to test this code. Any thoughts on how it might be improved?

No, that's fine. Since the stream is just a vanilla async generator, I don't have any better suggestions. 👍

carver · 2019-07-30T18:16:48Z

tests/p2p/test_multiplexer.py

+            bob_p2p_protocol = bob_multiplexer.get_protocol_by_name('p2p')
+            bob_second_protocol = bob_multiplexer.get_protocol_by_name('second')
+
+            alice_second_protocol.send_cmd(CommandA)


This surprised me: you can send the uninitialized command? I would have expected send_cmd(CommandA())

Is it just because CommandA happens to have no data? I assume most commands (besides something like Ping) would include some kind of data. If it usually requires including data, then supporting the API of sending an class may not be worth being able to drop the parentheses.

If you scroll up you can see that send_cmd is just a method I implemented on SecondProtocol to simplify sending different command types and isn't part of the core Protocol API.

Ah, right, I was thinking it would be something on every Protocol 👍

carver · 2019-07-30T18:21:33Z

tests/p2p/test_multiplexer.py

+            alice_p2p_protocol.send_ping()
+            alice_second_protocol.send_cmd(CommandB)
+            cmd, _ = await asyncio.wait_for(bob_p2p_stream.asend(None), timeout=0.1)
+            assert isinstance(cmd, Ping)


Very cool, this test succinctly identifies the point of the mux, and shows it working. 👍

carver · 2019-07-30T18:27:40Z

newsfragments/835.feature.rst

@@ -0,0 +1 @@
+Add ``p2p.multiplexer.Multiplexer`` for multiplexing the individual protocol messages for a devp2p connection.


Maybe a little hint for the people who otherwise have to look up multiplexing. Something like:

Suggested change

Add ``p2p.multiplexer.Multiplexer`` for multiplexing the individual protocol messages for a devp2p connection.

Add ``p2p.multiplexer.Multiplexer`` for combining the commands from different devp2p sub-protocols into a single network write stream, and split the incoming network stream into individually retrievable sub-protocol commands.

carver · 2019-07-30T19:08:38Z

p2p/multiplexer.py

+                cmd_id_cache[cmd_id] = base_protocol
+            else:
+                for protocol in protocols:
+                    if cmd_id < protocol.cmd_id_offset + protocol.cmd_length:


Something still feels off about the protocol knowing its own offset. Like we should have some JointProtocol that knows the offsets. It could do something like:

class JointProtocol: def get_protocol_command(self, joint_command_id): offset = self._get_nearest_offset(joint_command_id) protocol = self._get_protocol_at_offset(offset) protocol_command_index = joint_command_id - offset return protocol.cmd_by_id[protocol_command_index]

Then the protocol itself knows nothing about offsets from combining the protocols. Anyway, just a nice to have, and probably would trigger a big refactor. Also, I remember you saying something about this being especially tricky on the sending side. But it could be nice if you like it, and the change doesn't seem huge.

carver · 2019-07-30T20:07:58Z

p2p/multiplexer.py

+        self._base_protocol = base_protocol
+
+        # the sub-protocol instances
+        self._protocols = protocols


If we're going to keep the lookup by name, we should probably validate that they there are no duplicate names here.

carver · 2019-07-30T20:19:56Z

p2p/multiplexer.py

+            raise TypeError("Unknown protocol identifier: {protocol}")
+
+        if not self.has_protocol(protocol_class):
+            raise Exception(f"TODO: Unknown protocol '{protocol_class}'")


carver · 2019-07-30T20:20:11Z

p2p/multiplexer.py

+        return self._transport.is_closing
+
+    def close(self) -> None:
+        # TODO: maybe graceflly close streams?


Maybe move to an issue?

The token is actually closing the streams gracefully so I just removed the comment.

pipermerriam · 2019-07-31T20:42:19Z

Test failures are unrelated.

pipermerriam force-pushed the piper/implement-multiplexer branch 2 times, most recently from 1da082d to 7943ae9 Compare July 24, 2019 18:07

pipermerriam changed the title ~~Piper/implement multiplexer~~ Implement multiplexer Jul 24, 2019

pipermerriam requested a review from carver July 24, 2019 18:14

pipermerriam force-pushed the piper/implement-multiplexer branch from 7943ae9 to 3e1c980 Compare July 24, 2019 18:16

pipermerriam mentioned this pull request Jul 24, 2019

[WIP] extract Peer and Protocol concepts. #684

Closed

2 tasks

pipermerriam removed the request for review from carver July 24, 2019 20:15

pipermerriam force-pushed the piper/implement-multiplexer branch 2 times, most recently from c0db057 to 38b6185 Compare July 25, 2019 16:49

pipermerriam mentioned this pull request Jul 25, 2019

Use multiplexer in peer class #847

Merged

2 tasks

pipermerriam force-pushed the piper/implement-multiplexer branch from 38b6185 to 0be597a Compare July 25, 2019 20:07

pipermerriam requested a review from carver July 25, 2019 20:08

pipermerriam force-pushed the piper/implement-multiplexer branch 2 times, most recently from 7aad0ea to f19f1cb Compare July 25, 2019 20:47

pipermerriam mentioned this pull request Jul 26, 2019

Implement factories for generating devp2p protocols #850

Merged

2 tasks

pipermerriam force-pushed the piper/implement-multiplexer branch 2 times, most recently from c7afb06 to eea863d Compare July 27, 2019 02:29

Add multiplexer

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

e51cb7c

pipermerriam force-pushed the piper/implement-multiplexer branch from eea863d to 27e4920 Compare July 29, 2019 21:31

Improvem multiplexer APIs a bit

18e3b91

pipermerriam force-pushed the piper/implement-multiplexer branch from 27e4920 to 18e3b91 Compare July 29, 2019 23:12

carver reviewed Jul 30, 2019

View reviewed changes

carver approved these changes Jul 30, 2019

View reviewed changes

PR Feedback

fbaab3a

pipermerriam merged commit 2437583 into ethereum:master Jul 31, 2019

pipermerriam deleted the piper/implement-multiplexer branch July 31, 2019 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement multiplexer #835

Implement multiplexer #835

pipermerriam commented Jul 24, 2019 •

edited

Loading

pipermerriam commented Jul 24, 2019

carver commented Jul 26, 2019

pipermerriam commented Jul 26, 2019

pipermerriam commented Jul 26, 2019

pipermerriam commented Jul 27, 2019

carver commented Jul 30, 2019

carver left a comment

carver Jul 30, 2019

carver Jul 30, 2019

pipermerriam Jul 30, 2019

carver Jul 30, 2019

pipermerriam Jul 30, 2019

carver Jul 30, 2019

carver Jul 30, 2019

pipermerriam Jul 30, 2019

carver Jul 30, 2019

carver Jul 30, 2019

carver Jul 30, 2019

carver Jul 30, 2019

carver Jul 30, 2019

carver Jul 30, 2019

carver Jul 30, 2019

pipermerriam Jul 31, 2019

pipermerriam commented Jul 31, 2019

		@@ -0,0 +1 @@
		Add ``p2p.multiplexer.Multiplexer`` for multiplexing the individual protocol messages for a devp2p connection.

Implement multiplexer #835

Implement multiplexer #835

Conversation

pipermerriam commented Jul 24, 2019 • edited Loading

What was wrong?

How was it fixed?

To-Do

Cute Animal Picture

pipermerriam commented Jul 24, 2019

carver commented Jul 26, 2019

pipermerriam commented Jul 26, 2019

pipermerriam commented Jul 26, 2019

pipermerriam commented Jul 27, 2019

carver commented Jul 30, 2019

carver 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

pipermerriam commented Jul 31, 2019

pipermerriam commented Jul 24, 2019 •

edited

Loading