kim
commented
3 years ago I suppose the idea here is to allow an impl of ConnectionSentinel to perform
its own bookeeping. But then, it would also need to be notified of close events,
which is not entirely practical because:
a. those objects might be closed by simply getting dropped b. finalisation (eg. transmitting outstanding data) may, however, occur later (via waking) c. we need to account for both incoming and outgoing connections / streams, and potentially treat them differently when computing a decision
I would suggest to instead confine bookeeping to the library entirely, and pass
a reference to some kind of statistics structure to the decision making
function. Also note that we allow bidirectional streams to be used for
gossip/membership, although we initiate only unidirectional ones. A policy
decision for a large number of short-lived uni streams opened in rapid
succession is rather different to compute than a weighted decision over
longer-lived concurrent streams (which is, most likely, why we're seeing
max_concurrent_uni_streams to not have the desired effect).
So in summary, while abstracting over this concern would be useful in the longer term, I think that we should first have an implementation of a policy which proves to work in order to be able to tell what inputs (which are, necessarily, internals) it requires.
To solve the issue at hand (#678), I'd suggest to first expose some transport
parameters (there's a TODO for that), and lower max_concurrent_bidi_streams to
something like 5 (instead of the default 100). Gossip uni streams can simply be
dropped when they hit a rate limit (we employ governor already), while
membership messages (uni streams) need to result in a DISCONNECT. On top of
that, a simple "ban list" of PeerIds may also prove effective.
alexjg
A small RFC positing an interface for applications to decide whether a connection or stream should be accepted.
Signed-off-by: Alex Good