[discovery] Isolate gossip rate limiting #2135
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit restructures the delivery of gossip query related messages, such that they are delivered directly to the gossip syncers. Gossip query rate limiting was introduced in lightningnetwork#1824 on a per-peer basis. However, since all gossip query messages were being delivered in the main event loop, the end result is that one rate-limited peer could stall all other peers. In addition, since no other peers would be able to submit gossip-related messages through the blocked event loop, the back pressure would eventually rate limit the read handlers of all peers as well. The end result would be lengthy delays in reading messages related to htlc forwarding. The fix is to lift the delivery of gossip query messages outside of the main event loop. With this change, the rate limiting backpressure is delivered only to the intended peer.
This commit passes the peer's quit signal to the gossipSyncer when attempt to hand off gossip query messages. This allows a rate-limited peer's read handler to break out immediately, which would otherwise remain stuck until the rate-limited gossip syncer pulled the message.
cfromknecht
changed the title
halseth
reviewed
Nov 2, 2018
| // For messages in the known set of channel series queries, we'll | ||
| // dispatch the message directly to the gossipSyncer, and skip the main | ||
| // processing loop. | ||
| switch m := msg.(type) { |
There was a problem hiding this comment.
I would be more confident in this change if we instead created a separate event loop for gossip query messages that we handed them off to.
There was a problem hiding this comment.
If they all go through one event loop then we’re back to square one. Are you suggesting one event loop per gossip syncer?
There was a problem hiding this comment.
I was thinking one event loop per peer, but I see now that that might get very complex 🤔
There was a problem hiding this comment.
Yeah in some sense this change already has one event loop per peer (the readHandler) and the back pressure is now applied there directly
Roasbeef
approved these changes
Nov 2, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.