-
Notifications
You must be signed in to change notification settings - Fork 168
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
feat: pass forceCloseConnections option from cli #765
base: master
Are you sure you want to change the base?
feat: pass forceCloseConnections option from cli #765
Conversation
README.md
Outdated
| Set the plugin timeout | `-T` | `--plugin-timeout` | `FASTIFY_PLUGIN_TIMEOUT` | | ||
| Set forceCloseConnections option on fastify instance | `-f` | `--force-close-connections` | `FASTIFY_FORCE_CLOSE_CONNECTIONS` | | ||
| Defines the maximum payload, in bytes,<br>that the server is allowed to accept | | `--body-limit` | `FASTIFY_BODY_LIMIT` | | ||
| Set the maximum ms delay before forcefully closing pending requests after receiving SIGTERM or SIGINT signals; and uncaughtException or unhandledRejection errors (default: 500) | `-g` | `--close-grace-delay` | `FASTIFY_CLOSE_GRACE_DELAY` | |
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.
Why did you reformat the whole table? Can you only make the change needed?
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.
Ops, I believed was some linter. Instead was my IDE. Restored the untouched lines
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.
lgtm
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.
TBH I would set this config to true
by default because it may cause data loss in production
- As is: an error logging message
- With default
true
: a request that is dropped in the middle of processing
As a user, I would be more pissed of about the 2nd default scenario.
Can't we write in the log:
process forced end. There were 2 open connections. Checkout the `forceCloseConnections` option
I don't get it. If We can write additional infos on message, but this message could also shown in production during restart with "real" chrome connections opened: may be a false hint? |
Enabling the proxy for the forceCloseConnections option. When enabled, this option resolves this issue. Explained in detail also inside this other issue
Please note that the original option on fastify could be both
string
andboolean
. In fastify-cli I didn't find a smart way to manage it (it seems that's possible defining only one type in a structural way). I choose to use it as boolean/flag, leaving the only allowed string value (idle
) the default option.If there is a better standard solution, please point me in the right direction :)
Checklist
npm run test
andnpm run benchmark
and the Code of conduct