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

Update stream limits and connection limits #53

Merged
merged 10 commits into from
Apr 15, 2023
Merged

Conversation

p-shahi
Copy link
Member

@p-shahi p-shahi commented Apr 14, 2023

No description provided.

@vercel
Copy link

vercel bot commented Apr 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
universal-connectivity ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2023 4:47pm

@p-shahi p-shahi changed the title Update stream limits Update stream limits and connection limits Apr 15, 2023
Comment on lines +73 to +78
// these are set because we were seeing a lot of identify and identify push
// stream limits being hit
maxPushOutgoingStreams: 1000,
maxPushIncomingStreams: 1000,
maxInboundStreams: 1000,
maxOutboundStreams: 1000,
Copy link
Member

Choose a reason for hiding this comment

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

I am concerned by this. These limits are per connection, this means you're allowing 1000 identify streams to be open at the same time on each connection.

If you're seeing errors it sounds like something's not cleaning up the streams.

Maybe related: libp2p/js-libp2p#1424

Copy link
Member

Choose a reason for hiding this comment

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

Do what you have to to appease the demo gods though - this is more of a note to self to investigate.

Copy link
Member Author

@p-shahi p-shahi Apr 15, 2023

Choose a reason for hiding this comment

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

Yeah the upper bounds here are crazy, but we were hitting the limits quite often so set it arbitrarily high

Comment on lines 53 to 56
connectionManager: {
maxConnections: 200,
minConnections: 1,
maxConnections: 10,
minConnections: 50,
},
Copy link
Member

Choose a reason for hiding this comment

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

minConnections needs to be less than maxConnections.

Is there a reason you're overriding this? If not I'd just remove the whole connectionManager block.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed, I'm not sure why it was added. @maschad any idea?

@p-shahi p-shahi merged commit 9d8e341 into main Apr 15, 2023
1 check passed
@p-shahi p-shahi deleted the update-stream-limits branch April 20, 2023 04:01
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

3 participants