Commit cb1c9934 authored by Kirill Smelkov's avatar Kirill Smelkov

Connection: Adjust msg_id a bit so it behaves like stream_id in HTTP/2

This is 2020 edition of my original patch from 2016 ( dd3bb8b4 ).

It was described in my NEO/go article ( https://navytux.spb.ru/~kirr/neo.html )
in the paragraph quoted below:

    NEO/go shifts from thinking about protocol logic as RPC to thinking of it as
    more general network protocol and settles to provide general
    connection-oriented message exchange service[1] : whenever a message with new
    `msg_id` is sent, a new connection is established multiplexed on top of a
    single node-node TCP link. Then it is possible to send/receive arbitrary
    messages over back and forth until so established connection is closed. This
    works transparently to NEO/py who still thinks it operates in simple RPC mode
    because of the way messages are put on the wire and because simple RPC is
    subset of a general exchange.  The `neonet` module also provides `DialLink` and
    `ListenLink` primitives[2] that work similarly to standard Go `net.Dial` and
    `net.Listen` but wrap so created link into the multiplexing layer. What is
    actually done this way is very similar to HTTP/2 which also provides multiple
    general streams multiplexing on top of a single TCP connection ([3], [4]).
    However if connection ids (sent in place of `msg_id` on the wire) are assigned
    arbitrary, there could be a case when two nodes could try to initiate two new
    different connections to each other with the same connection id. To prevent
    such kind of conflict a simple rule to allocate connection ids either even or
    odd, depending on the role peer played while establishing the link, could be
    used. HTTP/2 takes similar approach[5] where `"Streams initiated by a client
    MUST use odd-numbered stream identifiers; those initiated by the server MUST
    use even-numbered stream identifiers."` with NEO/go doing the same
    corresponding to who was originally dialer and who was a listener. However it
    requires small patch to be applied on NEO/py side to increment `msg_id` by 2
    instead of 1.

    [1] https://lab.nexedi.com/kirr/neo/blob/463ef9ad/go/neo/neonet/connection.go
    [2] https://lab.nexedi.com/kirr/neo/blob/463ef9ad/go/neo/neonet/newlink.go
    [3] https://tools.ietf.org/html/rfc7540#section-5
    [4] https://http2.github.io/faq/#why-is-http2-multiplexed
    [5] https://tools.ietf.org/html/rfc7540#section-5.1.1

It can be named as "terrible", "irritating", "stupid" or "crazy", but the fact is:

- it does no harm to NEO/py and is backward-compatible: a NEO/py node
  without this patch can still successfully connect and interoperate to
  another NEO/py node with this patch.

- it is required for NEO/go to be able to interoperate with NEO/py.
  Both client and server parts of NEO/go use the same neonet module to exchange messages.

- NEO/go client is used by wendelin.core 2, which organizes access to on-ZODB
  ZBigFile data via WCFS filesystem implemented in Go.

So on one side this patch is small, simple and does not do any harm to NEO/py.
On the other side it is required for NEO/go and wendelin.core 2.

To me this clearly indicates that there should be NO GOOD REASON to reject
inclusion of this patch into NEO/py.

--------

My original patch from 2016 came with corresponding adjustments to neo/tests/testConnection.py
( dd3bb8b4 )
but commit f6eb02b4 (Remove packet timeouts; 2017-05-04) removed testConnection.py
completely and, if I understand correctly, did not add any other test to
compensate that. This way I'm not trying to restore my tests to
Connection neither.

Anyway, with this patch there is no regression to all other existing NEO/py tests.

--------

My original patch description from 2016 follows:

- even for server initiated streams
- odd  for client initiated streams

This way I will be able to use Pkt.msg_id as real stream_id in go's Conn
because with even / odd scheme there is no possibility for id conflicts
in between two peers.

/cc @romain, @tomo, @rafael, @arnau, @vpelletier, @klaus, @Tyagov
parent f2ea4be2
......@@ -316,7 +316,7 @@ class Connection(BaseConnection):
def __init__(self, event_manager, *args, **kw):
BaseConnection.__init__(self, event_manager, *args, **kw)
self.read_buf = ReadBuffer()
self.cur_id = 0
# NOTE .cur_id will be set in Server|Client to maintain `cur_id % 2 == const` invariant
self.aborted = False
self.uuid = None
self._queue = []
......@@ -347,11 +347,14 @@ class Connection(BaseConnection):
del self._timeout
except AttributeError:
self.client = True
self.cur_id |= 1 # client has `.cur_id % 2 == 1`
else:
assert self.client
def asServer(self):
self.server = True
# server has `.cur_id % 2 == 0`
self.cur_id = (self.cur_id + 1) & 0xfffffffe
def _closeClient(self):
if self.server:
......@@ -389,7 +392,7 @@ class Connection(BaseConnection):
def _getNextId(self):
next_id = self.cur_id
self.cur_id = (next_id + 1) & 0xffffffff
self.cur_id = (next_id + 2) & 0xffffffff
return next_id
def getTimeout(self):
......@@ -593,6 +596,7 @@ class ClientConnection(Connection):
"""A connection from this node to a remote node."""
client = True
cur_id = 1 # cur_id % 2 is 1 for client initiated "streams"
def __init__(self, app, handler, node):
self._ssl = app.ssl
......@@ -644,6 +648,7 @@ class ServerConnection(Connection):
"""A connection from a remote node to this node."""
server = True
cur_id = 0 # cur_id % 2 is 0 for server initated "streams"
def __init__(self, *args, **kw):
Connection.__init__(self, *args, **kw)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment