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.

Sign up for GitHub

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

Jump to bottom

feat: Update connectionManager to allow configuration of incoming and outgoing connection limits #1511

Closed
wants to merge 18 commits into from
Closed

feat: Update connectionManager to allow configuration of incoming and outgoing connection limits #1511

wants to merge 18 commits into from

Conversation

maschad
Copy link
Member

@maschad maschad commented Dec 5, 2022 •
edited
Loading

Closes #1508

@maschad maschad marked this pull request as draft December 5, 2022 20:16
maschad
maschad commented Dec 5, 2022
View reviewed changes
doc/LIMITS.md Outdated Show resolved Hide resolved
src/connection-manager/auto-dialler.ts Outdated Show resolved Hide resolved
@maschad maschad force-pushed the feat/seperate-limits-for-connections branch 2 times, most recently from 1f522e0 to d6b4fd3 Compare December 6, 2022 21:30
@maschad maschad marked this pull request as ready for review December 6, 2022 21:30
@maschad
Copy link
Member Author

maschad commented Dec 6, 2022

I've updated the docs should be ready to review now.

@maschad
Copy link
Member Author

maschad commented Dec 8, 2022

I also decided to include #1515 in this PR as well here

@maschad maschad mentioned this pull request Dec 8, 2022
Closed
@maschad maschad marked this pull request as draft December 13, 2022 16:31
@maschad
Copy link
Member Author

maschad commented Dec 13, 2022

It may not actually be useful to have a minimum inbound/outbound connection limit. In the case of a min inbound connection limit, this could force the connectionManager into a endless loop of dialing connections where that inbound limit is never met since each new connection would be an outbound connection. We should just have a minimum connection limit.

@maschad maschad force-pushed the feat/seperate-limits-for-connections branch from 7d17a3b to 2ec8606 Compare December 13, 2022 22:18
@maschad maschad marked this pull request as ready for review December 13, 2022 22:18
@maschad
Copy link
Member Author

maschad commented Dec 13, 2022

It may not actually be useful to have a minimum inbound/outbound connection limit. In the case of a min inbound connection limit, this could force the connectionManager into a endless loop of dialing connections where that inbound limit is never met since each new connection would be an outbound connection. We should just have a minimum connection limit.

This has been refactored. Ready for review @achingbrain

@maschad maschad force-pushed the feat/seperate-limits-for-connections branch 2 times, most recently from 6812ac1 to 24f7709 Compare December 16, 2022 03:10
@maschad maschad force-pushed the feat/seperate-limits-for-connections branch 2 times, most recently from 1c3484b to 72abe72 Compare January 4, 2023 19:14
@p-shahi p-shahi requested a review from achingbrain January 6, 2023 03:10
@maschad maschad force-pushed the feat/seperate-limits-for-connections branch from 72abe72 to 80dc00d Compare January 7, 2023 11:53
@maschad maschad mentioned this pull request Jan 7, 2023
Closed
@maschad
Copy link
Member Author

maschad commented Jan 7, 2023

The examples pubsub test seems to be flaky as it passes on my branch.

@maschad maschad force-pushed the feat/seperate-limits-for-connections branch from 80dc00d to 9344798 Compare January 16, 2023 17:35
@maschad maschad mentioned this pull request Jan 16, 2023
Merged
@p-shahi p-shahi requested a review from mpetrunic January 17, 2023 05:16
@p-shahi
Copy link
Member

p-shahi commented Jan 17, 2023

I also decided to include #1515 in this PR as well here

@maschad any reason for the force-pushes? I can't see what that commit references.

Also this is ready for review right, @mpetrunic would you be able to help review (requesting out of caution since not sure if achingbrain will get to it with Helia work)

@maschad maschad requested review from achingbrain and removed request for mpetrunic January 18, 2023 21:52
@maschad
Copy link
Member Author

maschad commented Jan 18, 2023

Please can you revert all of the formatting changes, there are many and they make it easy to miss the actual changes here. I've marked the ones I've spotted, there may be more.

I've reverted those changes, I believe we can still go ahead with the changes as explained here so should be ready for review now.

mpetrunic
mpetrunic suggested changes Jan 20, 2023
View reviewed changes
src/connection-manager/index.ts Outdated Show resolved Hide resolved
src/connection-manager/index.ts Outdated Show resolved Hide resolved
src/connection-manager/index.ts Outdated Show resolved Hide resolved
maschad added a commit to maschad/js-libp2p that referenced this pull request Jan 23, 2023
@maschad
fix: updated connection limits to filter for inbound/outbound (libp2p…
8e11bf7 
…#1511)
@maschad maschad requested review from mpetrunic and achingbrain and removed request for achingbrain and mpetrunic January 23, 2023 16:22
@maschad
Copy link
Member Author

maschad commented Jan 23, 2023

Thanks for pointing out those issues @mpetrunic I've made the updates. I would be interested in @achingbrain 's response to #1511 (comment) otherwise we should be good to go.

mpetrunic
mpetrunic reviewed Jan 24, 2023
View reviewed changes
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 small comments otherwise LGTM

src/connection-manager/index.ts Outdated Show resolved Hide resolved
src/connection-manager/index.ts Outdated Show resolved Hide resolved
@maschad maschad requested a review from mpetrunic January 24, 2023 14:06
@maschad maschad force-pushed the feat/seperate-limits-for-connections branch from 8e11bf7 to 9e75969 Compare January 24, 2023 14:31
mpetrunic
mpetrunic approved these changes Jan 24, 2023
View reviewed changes
Copy link
Member

@mpetrunic mpetrunic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let's wait for @achingbrain to take a look

@maschad maschad requested a review from achingbrain January 25, 2023 14:54
@maschad maschad mentioned this pull request Feb 1, 2023
Merged
@maschad
Copy link
Member Author

maschad commented Feb 7, 2023

Closing this as discussed with @achingbrain and @wemeetagain as consumers may have an idea of the maxConnections they want to maintain ahead of time but may only have a notion of a ratio of inbound to outbound as opposed to a fixed number. This could lead to issues with unwanted pruning of connections. For consumers who want this level of granularity they wish to pursue a custom ConnectionManager

@maschad maschad closed this Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpetrunic mpetrunic mpetrunic approved these changes

@achingbrain achingbrain Awaiting requested review from achingbrain

Assignees

@achingbrain achingbrain

Labels
None yet
Projects
js-libp2p
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Control inbound/outbound connection limits separately
4 participants
@maschad @p-shahi @achingbrain @mpetrunic