Skip to content

Conversation

nitschSB
Copy link
Contributor

@nitschSB nitschSB commented Mar 8, 2023

If vrt is running behind a reverse proxy sporadic 502 responses may occure due to incompatible timeouts.

This PR adds the capability to configure server timeouts.

More information at https://iximiuz.com/en/posts/reverse-proxy-http-keep-alive-and-502s/

@nitschSB nitschSB requested a review from pashidlos March 9, 2023 16:37
onApplicationBootstrap() {
const server: Server = this.refHost.httpAdapter.getHttpServer();
const { timeout, headersTimeout, keepAliveTimeout } = server;
server.timeout = parseInt(this.configService.get('SERVER_TIMEOUT', timeout.toString()));
Copy link
Member

Choose a reason for hiding this comment

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

could you check it's working when variable is not set?
might need default value for them of avoid assigning if it's undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure
I'm currently running this with an image i build myself.

line 17 is reading the defaults from node server and the second argument of this.configService.get assigns the default again, when the variable is not set

log from my server, after restart:

[Nest] 19  - 03/16/2023, 8:51:43 AM     LOG [RouterExplorer] Mapped {/projects/:id, DELETE} route +1ms
[Nest] 19  - 03/16/2023, 8:51:44 AM     LOG [AppService] timeout=120000
[Nest] 19  - 03/16/2023, 8:51:44 AM     LOG [AppService] headersTimeout=60000
[Nest] 19  - 03/16/2023, 8:51:44 AM     LOG [AppService] keepAliveTimeout=181000
[Nest] 19  - 03/16/2023, 8:51:44 AM     LOG [NestApplication] Nest application successfully started +0ms

timeout and headersTimeout is the default from node https://nodejs.org/docs/latest-v12.x/api/http.html#http_server_headerstimeout

keepAliveTimeout is overwritten by environment variable SERVER_KEEP_ALIVE_TIMEOUT=181000

Copy link
Member

@pashidlos pashidlos left a comment

Choose a reason for hiding this comment

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

LGTM!

@pashidlos pashidlos merged commit 441486f into master Mar 17, 2023
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.

2 participants