make message large message handling robust and noisy #189
Conversation
Stebalien
commented
Jun 6, 2019
Stebalien
commented
- Reject outgoing messages that are too large.
- Drop outgoing RPCs that are too large and log.
- Increase the max RPC size to 1MiB+64KiB (to allow for 1MiB messages).
1. Reject outgoing messages that are too large. 2. Drop outgoing RPCs that are too large and _log_. 3. Increase the max RPC size to 1MiB+64KiB (to allow for 1MiB messages).
Stebalien
force-pushed
the
fix/reliable-message-size
branch
from
85e8f8a
to
da3290e
Compare
|
travis fails with: |
pubsub.go
Outdated
| func (p *PubSub) Publish(topic string, data []byte) error { | ||
| if len(data) > 0 { |
There was a problem hiding this comment.
BUG: that should be
if len(data) > maxMessageSize
| @@ -70,11 +70,20 @@ func (fs *FloodSubRouter) Publish(from peer.ID, msg *pb.Message) { | |||
| continue | |||
| } | |||
|
|
|||
| if out.Size() > maxRPCSize { | |||
There was a problem hiding this comment.
Is this check really necessary here?
We already drop large messages on publish and can't read messages larger than maxRCPSize anyway.
There was a problem hiding this comment.
I guess it should be fine in floodsub. My worry is gossipsub.
There was a problem hiding this comment.
well, let's remove it from here as it's redundant, and rethink the gossipsub case.
There was a problem hiding this comment.
This can't hurt and future proofs us against, e.g., batching too many messages in a single RPC and then silently failing when the remote side drops it.
| ctl := out.GetControl() | ||
| if ctl != nil { | ||
| gs.pushControl(p, ctl) | ||
| if out.Size() > maxRPCSize { |
There was a problem hiding this comment.
here too, this check seems unnecessary.
There was a problem hiding this comment.
I'm worried we could add too much gossip. That's why I have that check. But I can remove it if you think it's overkill.
There was a problem hiding this comment.
but the check is done before we even add the gossip!
There was a problem hiding this comment.
ok, this check is reasonable on second thought, if we are adding too much gossip (over 64KB) we probably want to know.
