fix: Make identify-push autodial smarter #3038
Closed
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
shobit000
changed the title
achingbrain
requested changes
Mar 6, 2025
There was a problem hiding this comment.
This does not fix the issue in #2389. Multiple streams will still be opened if multiple protocols are registered/unregistered one after another.
Worse, the proposed fix would introduce a bug whereby busy nodes with lots of connections might succeed in sending push updates but nodes with only a few connections would not.
|
Closing as all this does is introduce some whitespace changes and unused fields. It does not fix the issue. |
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.
Title
Enhance Identify Push to Avoid Connection Drops Due to Rate Limiter Violations
Description
This PR addresses Issue #2389, which describes a problem where multiple protocols being enabled simultaneously trigger multiple identify-push requests. This can overwhelm receiving peers and result in connection drops due to rate limiter violations.
Changes Made
Introduced pLimit for concurrency control.
Batched identify-push requests to avoid overwhelming peers.
Modified IdentifyPush class in packages/protocol-identify/src/identify-push.ts.
Implemented the rate limiting and batching logic in the push method.
Updated doc/SERVICES.md to reflect changes in the identify-push process.
Documented the new configuration options for IdentifyPush.
Updated packages/protocol-identify/test/push.spec.ts to include tests for the new rate limiting and batching logic.
Added a test case to ensure the number of concurrent identify-push requests is limited.
Files Changed
packages/protocol-identify/src/identify-push.ts
doc/SERVICES.md
packages/protocol-identify/test/push.spec.ts
Additional Information
Issue Link: Issue #2389
Repository: libp2p/js-libp2p