-
Notifications
You must be signed in to change notification settings - Fork 336
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(web-server): reconnect window can be manually set but keeps a 60s default value #10754
Conversation
packages/server/server.ts
Outdated
@@ -34,10 +34,13 @@ tracer.init({ | |||
tracer.use('ioredis').use('http').use('pg') | |||
|
|||
process.on('SIGTERM', async (signal) => { | |||
const RECONNECT_WINDOW = process.env.WEB_SERVER_RECONNECT_WINDOW | |||
? parseInt(process.env.WEB_SERVER_RECONNECT_WINDOW, 10) * 1000 | |||
: 60_000 // ms |
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.
-1 We should move this top level. That way it's evaluated already on first importing the file and a NUMBER_FORMAT_EXCEPTION
would interrupt startup. It's better than finding invalid environment variables only at runtime.
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.
Agree! Where is that? 🤔
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.
Just at the top of the file is fine, l27.
.env.example
Outdated
@@ -54,6 +54,10 @@ FILE_STORE_PROVIDER='local' | |||
# Google Cloud credentials are required to use FILE_STORE_PROVIDER='gcs' | |||
# GOOGLE_GCS_BUCKET='BUCKET_NAME' | |||
|
|||
# WEB SERVER | |||
# Time window, in seconds, used by the web server to kick its users off. If empty, it has a default value. |
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.
+1 Maybe mention the default value? "If empty it defaults to 60s."
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.
Yeah, I wanted to avoid that, just in case we changed the default value one day. That said, minor change and clearer information so I'm adding that
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 forgot that JavaScript does not throw in parseInt. So validation would involve a bit more. It's ok.
Description
Allows setting a custom reconnect window for the web server, so it can gracefully stop in environments where 60s grace period isn't allowed.
Demo