transport: add GatedMaListener type - #3186
Conversation
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
brief review. Overall I like being explicit here.
| // UpgradeListener upgrades the passed multiaddr-net listener into a full libp2p-transport listener. | ||
| // | ||
| // Deprecated: Use UpgradeGatedManetListener(upgrader.GateManetListener(manet.Listener)) instead. | ||
| UpgradeListener(Transport, manet.Listener) Listener |
There was a problem hiding this comment.
Is anyone besides us using this? Should we just remove it?
| l.mx.Lock() | ||
| l.scopes[conn] = scope | ||
| // TODO: This should be 10 seconds? | ||
| time.AfterFunc(time.Minute, func() { |
There was a problem hiding this comment.
Is this in case the connection is closed or dropped? This is kind of hacky
a3eadf2 to
78cd22b
Compare
sukunrt
changed the title
ae94a2d to
6b3b600
Compare
6b3b600 to
512a7d5
Compare
This introduces a new GatedMaListener type which gates conns accepted from a manet.Listener with a gater and creates the rcmgr scope for it. Explicitly passing the scope allows for many guardrails that the previous interface assertion didn't. This breaks the previous responsibility of the upgradeListener method into two, one gating the connection initially, and the other upgrading the connection with a security and muxer selection. This split makes it easy to gate the connection with the resource manager as early as possible. This is especially true for websocket because we want to gate the connection just after the TCP connection is established, and not after the tls handshake + websocket upgrade is completed.
512a7d5 to
f1b1ee9
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
| gmal = upgrader.GateMaListener(mal) |
There was a problem hiding this comment.
Note: This changes the address reported to the resource manager and connection gater from:
/ip4/a.b.c.d/tcp/xx/ws to /ip4/a.b.c.d/tcp/xx
We are not providing the /ws component anymore. This is the same behavior as what the shared tcp listener does as it doesn't know whether the connection will be a websocket or tcp one.
This is easy enough to fix, but that would make the behavior between the shared tcp listener and unshared one inconsistent.
I don't think the resource manager cares, as we don't have a limit of connections per transport.
The conn gater might care about this. If that's the case, we can move the conngater.InterceptAccept into GateMaListener and have the gatedMaListener only manage resource manager scope.
There was a problem hiding this comment.
I don't think I understand. Are we not including the ws component in the
connection's Multiaddr methods? We probably should since it's often useful for
debugging and understanding where a connection came from.
There was a problem hiding this comment.
We'll need a method on the Scope / Rcmgr for this. We don't know whether it'll be a tcp or a ws connection just when we've accepted the tcp connection.
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
Looks good. Just one question around the multiaddr not including "ws". I think it probably should, if only to help debugging.
|
|
||
| if t.enableMetrics { | ||
| list = newTracingListener(&tcpListener{list, 0}, t.metricsCollector) | ||
| // TODO: Fix this: The tcpListener wrapping should happen on both enableMetrics and disabledMetrics path |
There was a problem hiding this comment.
Why would it happen on the disable metrics path?
There was a problem hiding this comment.
there are two wrappings. One is for metrics. That shouldn't happen on metrics path. The other is for setting socket options like SoLinger(0). That should happen on both parts.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| gmal = upgrader.GateMaListener(mal) |
There was a problem hiding this comment.
I don't think I understand. Are we not including the ws component in the
connection's Multiaddr methods? We probably should since it's often useful for
debugging and understanding where a connection came from.
3 participants
This introduces a new GatedMaListener type which gates conns
accepted from a manet.Listener with a gater and creates the rcmgr
scope for it. Explicitly passing the scope allows for many guardrails
that the previous interface assertion didn't.
This breaks the previous responsibility of the upgradeListener method
into two, one gating the connection initially, and the other upgrading
the connection with a security and muxer selection.
This split makes it easy to gate the connection with the resource
manager as early as possible. This is especially true for websocket
because we want to gate the connection just after the TCP connection is
established, and not after the tls handshake + websocket upgrade is
completed.
fixes: #3182