-
Notifications
You must be signed in to change notification settings - Fork 38
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
chore(server): Set keep-alive and headers timeouts on server start #333
chore(server): Set keep-alive and headers timeouts on server start #333
Conversation
Thanks for the PR! I already expose the httpServer on
Do you know if most folks change these values from the defaults? If so, your change probably makes sense. |
That's what we are currently doing and setting those values stopped our random 502 issue. It's also what we do elsewhere at Indigo. I'm not sure if this is what everyone does but 30s+ has been standard everywhere I've worked. My goal was to make it simpler and easier for others to avoid the same performance problems that we encountered. |
src/core/config.ts
Outdated
WARTHOG_VALIDATE_RESOLVERS: 'false' | ||
WARTHOG_VALIDATE_RESOLVERS: 'false', | ||
WARTHOG_KEEP_ALIVE_TIMEOUT_MS: 30000, | ||
WARTHOG_HEADERS_TIMEOUT_MS: 5000 |
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 think since you changed this to not be an offset, we'd need to set this to a value larger than WARTHOG_KEEP_ALIVE_TIMEOUT_MS
, right?
In looking at these pages:
- https://shuheikagawa.com/blog/2019/04/25/keep-alive-timeout/
- https://docs.aws.amazon.com/elasticloadbalancing/latest/application/application-load-balancers.html#connection-idle-timeout
- https://nodejs.org/api/http.html#http_server_headerstimeout
It seems both of these values need to be > 60. Should we do something like 61000
and 62000
?
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.
It's more about headersTimeout > keepAliveTimeout
. 30s is a good keep alive (though if you want it to be 60s by default I can certainly make that change), but the key part of this is to ensure that the headers time out is greater than the keep alive.
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 pushed up the change to make headers timeout 60s, that was an oversight in my last commit.
Looks great, thanks 👍 |
🎉 This PR is included in version 2.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.