...
 
Commits (1)
  • Kirill Smelkov's avatar
    Connection: Adjust msg_id a bit so it behaves like stream_id in HTTP/2 · 06d582e5
    Kirill Smelkov authored
    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 text quoted below:
    
        Then comes the link layer which provides service to exchange messages over
        network. In current NEO/py every message has `msg_id` field, that similarly to
        ZEO/py marks a request with serial number with requester then waiting for
        corresponding answer to come back with the same message id. Even though there
        might be several reply messages coming back to a single request, as e.g. NEO/py
        asynchronous replication code[0], this approach is still similar to ZEO/py
        remote procedure call (RPC) model because of single request semantic. One of
        the places where this limitation shows is the same replicator code where
        transactions metadata is fetched first with first series of RPC calls, and only
        then object data is fetched with the second series of RPC calls. This could be
        not very good e.g. in case when there is a lot of transactions/data to
        synchronize, because 1) it puts assumption on, and so constraints, the storage
        backend model on how data is stored (separate SQL tables for metadata and
        data), and 2) no data will be synchronized at all until all transactions are
        synchronized first. The second point prevents for example the syncing storage
        in turn to provide, even if read-only, service for the already fetched data.
        What would be maybe more useful is for requester to send request that it wants
        to fetch ZODB data in `tid_min..tid_max` range and then the sender sending
        intermixed stream of metadata/data in zodbdump-like format.
    
        Keeping in mind this, and other examples, 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.
    
        NEO/py currently explicitly specifies `msg_id` for an answer in only limited
        set of cases, by default assuming a reply comes to the last received message
        whose `msg_id` it remembers globally per TCP-link. This approach is error-prone
        and cannot generally work in cases where several simultaneous requests are
        received over single link. This way NEO/go does not maintain any such global
        per-link knowledge and handles every request by always explicitly using
        corresponding connection object created at request reception time.
    
        [0] https://lab.nexedi.com/kirr/neo/blob/463ef9ad/neo/storage/replicator.py
        [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 criticized, 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
    06d582e5
......@@ -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)
......