Skip to content
This repository was archived by the owner on Aug 29, 2023. It is now read-only.

feat: close server on maxConnections#218

Merged
wemeetagain merged 5 commits intolibp2p:masterfrom
dapplion:dapplion/close-server
Jan 20, 2023
Merged

feat: close server on maxConnections#218
wemeetagain merged 5 commits intolibp2p:masterfrom
dapplion:dapplion/close-server

Conversation

@dapplion
Copy link
Copy Markdown
Contributor

@dapplion dapplion commented Oct 7, 2022

More resilient strategy to #213 which blocks new connections at the OS layer instead of at the app layer. Both features should exist since each has different tradeoffs. The approach of this PR is necessary for critical apps like Lodestar in highly adversarial environments. #213 is good for less critical situations, where the added complexity is not justified.

@achingbrain
Copy link
Copy Markdown
Member

achingbrain commented Oct 7, 2022

This is critical behaviour - please can you add some tests to ensure regressions to not creep in.

@dapplion
Copy link
Copy Markdown
Contributor Author

dapplion commented Oct 7, 2022

This is critical behaviour - please can you add some tests to ensure regressions to not creep in.

@achingbrain
Copy link
Copy Markdown
Member

achingbrain commented Oct 11, 2022 •
edited
Loading

#214 is merged, this branch needs a rebase or otherwise master merging into it.

@dapplion dapplion force-pushed the dapplion/close-server branch 3 times, most recently from ae1ffdb to 507c19d Compare October 14, 2022 19:03
@dapplion
Copy link
Copy Markdown
Contributor Author

dapplion commented Oct 18, 2022

@achingbrain Rebased + added unit test

wemeetagain
wemeetagain reviewed Oct 28, 2022
View reviewed changes
Comment thread src/index.ts Outdated
dapplion
dapplion commented Oct 28, 2022
View reviewed changes
Comment thread src/listener.ts Outdated

export interface LimitServerConnectionsOpts {
acceptBelow: number
rejectAbove: number
Copy link
Copy Markdown
Contributor Author

@dapplion dapplion Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
rejectAbove: number
/** Server listens once connection count is less than `listenBelow` */
listenBelow: number
/** Close server once connection count is greater or equal than `closeAbove` */
closeAt: number

Copy link
Copy Markdown
Contributor Author

@dapplion dapplion Oct 28, 2022

Choose a reason for hiding this comment

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

On constructor sanity check that closeAbove >= listenBelow

dapplion
dapplion commented Oct 28, 2022
View reviewed changes
Comment thread src/listener.ts Outdated
socketInactivityTimeout?: number
socketCloseTimeout?: number
maxConnections?: number
limitServerConnections?: LimitServerConnectionsOpts
Copy link
Copy Markdown
Contributor Author

@dapplion dapplion Oct 28, 2022

Choose a reason for hiding this comment

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

Suggested change
limitServerConnections?: LimitServerConnectionsOpts
closeServerOnMaxConnections?: LimitServerConnectionsOpts

@mpetrunic mpetrunic added the need/author-input Needs input from the original author label Nov 8, 2022
@p-shahi
Copy link
Copy Markdown
Member

p-shahi commented Nov 22, 2022

@dapplion Any updates on this?

@dapplion
Copy link
Copy Markdown
Contributor Author

dapplion commented Dec 4, 2022

@dapplion Any updates on this?

Good to go

@dapplion dapplion force-pushed the dapplion/close-server branch from 79bea7a to b155bf6 Compare December 4, 2022 11:02
@mpetrunic
Copy link
Copy Markdown
Member

mpetrunic commented Dec 5, 2022

@dapplion Any updates on this?

Good to go

There is a type error (CI failling)

@p-shahi
Copy link
Copy Markdown
Member

p-shahi commented Dec 6, 2022

@achingbrain do we want this in 0.41.0 ?

@p-shahi
Copy link
Copy Markdown
Member

p-shahi commented Jan 10, 2023

Is this good to merge @dapplion @mpetrunic ?

@achingbrain
Copy link
Copy Markdown
Member

achingbrain commented Jan 17, 2023

@wemeetagain is going to test this change under load and merge it if all is well.

@wemeetagain
Copy link
Copy Markdown
Member

wemeetagain commented Jan 20, 2023

Ran this on a lodestar node, it works well.

@wemeetagain wemeetagain merged commit bff54fa into libp2p:master Jan 20, 2023
@dapplion dapplion deleted the dapplion/close-server branch January 21, 2023 04:31
github-actions Bot pushed a commit that referenced this pull request Jan 21, 2023
@semantic-release-bot
chore(release): 6.1.0 [skip ci]
07a03ac 
## [6.1.0](v6.0.9...v6.1.0) (2023-01-21)

### Features

* close server on maxConnections ([#218](#218)) ([bff54fa](bff54fa))

### Bug Fixes

* specify host explicitly in node tests ([#247](#247)) ([d7e5a69](d7e5a69))
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 21, 2023

🎉 This PR is included in version 6.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wemeetagain wemeetagain mentioned this pull request Jan 21, 2023
Merged
@achingbrain achingbrain mentioned this pull request Oct 23, 2024
Merged
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@wemeetagain wemeetagain wemeetagain approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

need/author-input Needs input from the original author released

Projects

js-libp2p
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dapplion @achingbrain @p-shahi @mpetrunic @wemeetagain
⚡