Exit gracefully on SIGINT #1356
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe update enhances the application's robustness by adding a 'SIGINT' process event listener in Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (1)
- rtl.js (1 hunks)
Additional comments: 2
rtl.js (2)
- 11-11: The
appvariable is correctly instantiated fromApp, but ensure thatApp's constructor and methods are designed to handle or propagate errors effectively, especially sinceapp.getApp()is used to create the server.- 11-11: The server setup and listening logic are correctly implemented. However, ensure that
common.hostandcommon.portare validated for valid values before using them to avoid potential runtime errors.
rtl.js
Outdated
| process.on('SIGINT', () => { | ||
| process.exit(); | ||
| }); |
There was a problem hiding this comment.
The SIGINT handler exits the process immediately without any cleanup. Consider gracefully shutting down connections or performing necessary cleanup before exiting.
process.on('SIGINT', () => {
+ // Perform necessary cleanup
+ console.log('Gracefully shutting down');
+ server.close(() => {
+ process.exit();
+ });
- process.exit();
});Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| process.on('SIGINT', () => { | |
| process.exit(); | |
| }); | |
| process.on('SIGINT', () => { | |
| // Perform necessary cleanup | |
| console.log('Gracefully shutting down'); | |
| server.close(() => { | |
| process.exit(); | |
| }); | |
| }); |
| process.on('SIGINT', () => { | ||
| process.exit(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Variable assignments for logger, common, and wsServer are redundant. Directly use the imported modules where needed or clarify the intention behind reassigning them to constants with the same name.
- const logger = Logger;
- const common = Common;
- const wsServer = WSServer;
+ // If reassignment is not necessary, consider using the imported modules directly.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
The error handling in onError function does not differentiate between critical and recoverable errors. All errors lead to process.exit(1), which might not be necessary for all cases. Consider implementing more nuanced error handling or recovery strategies.
The WebSocket server is mounted on the HTTP server without explicit error handling for WebSocket-specific errors. Consider adding error handling for the WebSocket server to ensure robustness.
|
I tried the code review bots suggestion, but it didn't fix the problem and still blocks stopping the service https://stackoverflow.com/questions/14626636/how-do-i-shutdown-a-node-js-https-server-immediately I think server.close() just stops the server accepting new connections but keeps alive any existing connections. Anyways, up to you if you want to accept this change. I am using it to run RTL in systemd in docker. |
* rm .DS_Store * Add watchfrontenddev command for npm * Fix toggle issues in sidenav (pinning and on page refresh) * Add copy-to-clipboard fallback if navigator.clipboard is not available (#1336) * add copy-to-clipboard fallback if navigator.clipboard is not available * amend copy fallback * clipboard copy lint fixes and frontend build * fix: add missing boltz state `transaction.lockupFailed` (#1349) * fix: boltzd docs link (#1354) * exit gracefully (#1356) * allow for eclair updated relayed audit format (#1363) * feat: add boltz service to cln (#1352) * lint fix * Request Params Cleanup * cln: Boltz auto-send (#1366) * Bug-fix (CLN Boltz): Hide claim tx id and routing fee for non-zero conf reverse swap * cln: Boltz auto-send - Added auto send option for Swap In - Checking compatiblity with v2.0.0 and above * Test import fixes * Update help.component.ts (#1379) Fixed broken link under "Help" -> "Node Settings" * Backend config fix (#1382) * Updating Common Application Configuration * Fixed get RTL Conf * Update Application Settings * application and settings case change * Unified config models * Default node update * 2FA and Password reset * Final application settings update * Config Settings and Authentication case fixed * Node Setting Fix * Fiat currency Symbol fix * CLN: Fiat symbol fix * All: Fiat symbol fix * Update node settings * Services UI fix * CLN: Removed child node settings * All: Removed child node settings * Test fixes * mempool links for onchain information (#1383) * Tests fix Tests fix * UI for Block Explorer Configuration (#1385) * Bump fee with mempool information (#1386) * Mempool openchannel minfee (#1388) Open channel model block if min fee is higher * Show error on login screen if rune is incorrect and getinfo throws error (#1391) * cln: Removed channel lookup call for update policy (#1392) * ECL: On-chain Transactions, Invoice and Payments pagination (#1393) Done most of the UI changes to accommodate pagination on transactions, payments and invoices tables but true pagination cannot be implemented till total number of records are missing from the API response. Once the issue ACINQ/eclair#2855 is fixed, I will uncomment pagination changes in the frontend. * lnd: Onchain CPFP (#1394) - UTXO label bug fix - Warning on utxo label for "sweep" in text. * Bug fixes after testing * Testing bug fixes (#1401) * Bug fix 2: lnd: Link channel point to explorer and show fee on close channel too * lnd: explorer link on pending channels * Node lookup link on view channel peer pubkey * Testing bug fixes (#1402) * Bug fix 2: lnd: Link channel point to explorer and show fee on close channel too * lnd: explorer link on pending channels * Node lookup link on view channel peer pubkey * test fixes * ng update to v18.0.x * Updating install with --legacy-peer-deps --------- Co-authored-by: Grzegorz Kućmierz <[email protected]> Co-authored-by: lacksfish <[email protected]> Co-authored-by: jackstar12 <[email protected]> Co-authored-by: Kilian <[email protected]> Co-authored-by: Taylor King <[email protected]> Co-authored-by: Fishcake <[email protected]> Co-authored-by: Ant <[email protected]>
I am trying to run this in docker under systemd and was getting failures trying to restart the job. Before ctrl + c would not kill the task successfully when running under docker, this fixes that as well.
Summary by CodeRabbit