-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
fix: gracefully terminate connections when closing http server #5786
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
d2bf447
to
7a1e435
Compare
@@ -106,8 +99,7 @@ export class RestApiServer { | |||
server.addHook("onError", async (req, _res, err) => { | |||
// Don't log ErrorAborted errors, they happen on node shutdown and are not useful | |||
// Don't log NodeISSyncing errors, they happen very frequently while syncing and the validator polls duties | |||
// Don't log eventstream aborted errors if server instance is being closed on node shutdown | |||
if (err instanceof ErrorAborted || err instanceof NodeIsSyncing || this.status === Status.Closed) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this in #5330 to avoid noisy errors on shutdown, however eventstream aborts are now properly handled and no more error are logged on shutdown.
Since we attempt to gracefully close connections now, it is highly unlikely to log any errors on shutdown since we should only get ErrorAborted
which is already checked and not logged. Any error that is not caught here is unexpected and should be logged.
3db6051
to
49583c1
Compare
49583c1
to
485a996
Compare
1cd9111
to
ff70972
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post merge review. This is great!!!
🎉 This PR is included in v1.10.0 🎉 |
Motivation
While looking into #5783 I noticed that our REST API server is not really "friendly" to clients, it abruptly terminates connections.
We should give the beacon node some time to complete pending requests and only forcefully close connections if timeout is reached. This improves client experience and prevents potential data corruption if request is writing data.
Description
Gracefully close HTTP server by waiting for all connections to drain until timeout
Connection: close
headerFurther considerations
It might make sense to close server immediately to stop accepting new connections.
Right now, what happens is that while terminating, new connections are just force closed by destroying the socket.
lodestar/packages/beacon-node/src/api/rest/activeSockets.ts
Lines 28 to 29 in 485a996
This causes
curl: (56) Recv failure: Connection reset by peer
error on the client.If the server is closed immediately it will cause
curl: (7) Failed to connect to localhost port 9596 after 0 ms: Connection refused
on the client as server stops listening for connections.The difference is likely insignificant in our case as the shutdown period is really short and the beacon node generally does not receive that many requests per second and in both cases the client will receive an error.
This has been discussed in http-terminator repository already gajus/http-terminator#22, but since our current and now updated implementation follows that of http-terminator closely, I kept the implementation to close server after terminating sockets.