Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LucaRainone
Copy link

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 and boolean. 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

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` |
Copy link
Member

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?

Copy link
Author

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

@LucaRainone LucaRainone deleted the feat/option-force-close-connections branch September 28, 2024 07:29
@LucaRainone LucaRainone restored the feat/option-force-close-connections branch September 28, 2024 07:30
@LucaRainone LucaRainone reopened this Sep 28, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@Eomm Eomm left a 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

@LucaRainone
Copy link
Author

TBH I would set this config to true by default because it may cause data loss in production

I don't get it.
If forceCloseConnections is true then it closes all pending connections regardless their status, causing possibly data loss. That's why it's not the default value.

If forceCloseConnections is not set in fastify-cli then it's not passed at all to fastify instance, keeping the default value of fastify option (that's idle or false depending by the server, so it tries a graceful shutdown limiting the data loss).

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?

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.

error occurs when writting files inside application directory with 'fastify start --watch'
3 participants