-
Notifications
You must be signed in to change notification settings - Fork 445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: prune connections based on stream counts and direction #2521
Conversation
Changes how we sort connections to prune to be more than just tags and age. Now we'll sort by tags, the number of open streams, direction and age. This should choose idle connections without streams to close over those that are in use.
aca8859
to
53f1bc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume that this insight came from interaction with other helia nodes, nevertheless would like to hear the rationale behind the changes.
}, {}) | ||
}) | ||
|
||
it('should sort connections for pruning', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the succinctness of this test but I do feel for readability it is at times better to be more explicit (at the expense of repetition). So a test would probably read as:
older connections with streams should be closed before tagged connections for example.
Otherwise debugging the sorting algorithm could become a bit cumbersome.
If we do something like a DHT query, we open a connection to a peer, open a The older connections are less useful now, since that peer plays no further part in the query. The more useful connections are the ones we're trying to open to continue the query - these are the newest connections so right now would be the ones that get closed by the pruner, almost immediately after being opened. Oops. Instead, we should look for connections that have no open streams - these are safe(r) to close because no protocol is using them. |
Thanks for the clarification |
Changes how we sort connections to prune to be more than just tags and age.
Now we'll sort by tags, the number of open streams, direction and age.
This should choose idle connections without streams to close over those that are in use.
Change checklist