feat: close server on maxConnections #218
Conversation
|
This is critical behaviour - please can you add some tests to ensure regressions to not creep in. |
|
|
#214 is merged, this branch needs a rebase or otherwise master merging into it. |
dapplion
force-pushed
the
dapplion/close-server
branch
3 times, most recently
from
ae1ffdb
to
507c19d
Compare
|
@achingbrain Rebased + added unit test |
src/listener.ts
Outdated
| @@ -25,15 +26,27 @@ async function attemptClose (maConn: MultiaddrConnection) { | |||
| } | |||
| } | |||
|
|
|||
| export interface LimitServerConnectionsOpts { | |||
| acceptBelow: number | |||
| rejectAbove: number | |||
There was a problem hiding this comment.
| 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 |
There was a problem hiding this comment.
On constructor sanity check that closeAbove >= listenBelow
src/listener.ts
Outdated
| interface Context extends TCPCreateListenerOptions { | ||
| handler?: (conn: Connection) => void | ||
| upgrader: Upgrader | ||
| socketInactivityTimeout?: number | ||
| socketCloseTimeout?: number | ||
| maxConnections?: number | ||
| limitServerConnections?: LimitServerConnectionsOpts |
There was a problem hiding this comment.
| limitServerConnections?: LimitServerConnectionsOpts | |
| closeServerOnMaxConnections?: LimitServerConnectionsOpts |
|
@dapplion Any updates on this? |
Good to go |
dapplion
force-pushed
the
dapplion/close-server
branch
from
79bea7a
to
b155bf6
Compare
There is a type error (CI failling) |
|
@achingbrain do we want this in 0.41.0 ? |
|
Is this good to merge @dapplion @mpetrunic ? |
|
@wemeetagain is going to test this change under load and merge it if all is well. |
|
Ran this on a lodestar node, it works well. |
|
🎉 This PR is included in version 6.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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.