Skip to content
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

Merged
merged 2 commits into from
May 7, 2024

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented May 2, 2024

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

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@achingbrain achingbrain requested a review from a team as a code owner May 2, 2024 17:47
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.
Copy link
Member

@maschad maschad left a 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 () => {
Copy link
Member

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.

@achingbrain
Copy link
Member Author

would like to hear the rationale behind the changes.

If we do something like a DHT query, we open a connection to a peer, open a /ipfs/kad/1.0.0 stream, send a message and read the response. We do this repeatedly and eventually hit the max connections limit. The pruner tries to close some connections to bring us back under the limit while we're trying to continue the query.

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.

@maschad
Copy link
Member

maschad commented May 3, 2024

Thanks for the clarification

@achingbrain achingbrain merged commit 8e36fc5 into main May 7, 2024
22 checks passed
@achingbrain achingbrain deleted the fix/prune-based-on-streams branch May 7, 2024 10:24
@achingbrain achingbrain mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants