feat: support timeout && drop node 14 support #6
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6 +/- ##
=========================================
Coverage ? 94.11%
=========================================
Files ? 2
Lines ? 51
Branches ? 17
=========================================
Hits ? 48
Misses ? 3
Partials ? 0
Continue to review full report at Codecov.
|
atian25
changed the title
|
ci 只留 node >=14 即可 |
WalkthroughThis pull request introduces significant updates to the Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant GracefulProcess as Graceful Process
participant BeforeExit as Before Exit Handler
participant Logger as Logger
App->>GracefulProcess: Initiate exit
GracefulProcess->>BeforeExit: Call with timeout
alt Exit within timeout
BeforeExit-->>GracefulProcess: Resolve successfully
GracefulProcess->>Logger: Log success
GracefulProcess->>App: Exit process
else Exit fails or times out
BeforeExit-->>GracefulProcess: Reject or timeout
GracefulProcess->>Logger: Log error
GracefulProcess->>App: Exit process
end
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
exit.js (1)
7-8: Simplify the assertion conditionThe assertion condition can be simplified using optional chaining as suggested by static analysis.
-assert(!beforeExit || typeof beforeExit === 'function', 'beforeExit only support function'); +assert(beforeExit?.constructor === Function, 'beforeExit only support function');.github/workflows/nodejs.yml (1)
16-17: Document the CODECOV_TOKEN requirementAdd documentation about the required CODECOV_TOKEN secret in the repository.
Add a note in the README.md about setting up the CODECOV_TOKEN secret for CI.
test/index.test.js (2)
14-18: Consider enhancing the killChild helper function.While the function effectively encapsulates the process termination logic, consider these improvements:
- Use debug logging instead of console.log to avoid polluting test output
- Add error handling for fkill failures
+const debug = require('debug')('graceful-process:test'); + async function killChild(child, force) { - console.log('killing %s', child.proc.pid); + debug('killing %s', child.proc.pid); try { await fkill(child.proc.pid, { force, silent: true }); - console.log('killed %s', child.proc.pid); + debug('killed %s', child.proc.pid); + } catch (err) { + debug('failed to kill %s: %s', child.proc.pid, err.message); + throw err; + } }
22-38: Consider improving test structure and constants.The test cases follow a good pattern but could benefit from:
- Extracting magic numbers (sleep durations) to named constants
- Adding comments to explain platform-specific behaviors
+// Test timing constants +const PROCESS_START_WAIT = waitStart; +const KILL_WAIT = 1000; +const TERM_SIGNAL_WAIT = 2000; + +// Platform specific signal handling +const isWindows = process.platform === 'win32'; + describe('cluster', () => { it('should workers auto exit when master kill by SIGKILL', async () => { // ... test code ... - await sleep(1000); + await sleep(KILL_WAIT); // ... test code ... }); it('should workers auto exit when master kill by SIGTERM', async () => { // ... test code ... - await sleep(2000); + await sleep(TERM_SIGNAL_WAIT); + // Windows doesn't support SIGTERM like Unix systems if (process.platform !== 'win32') { // ... test code ... } }); });Also applies to: 56-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.autod.conf.js(0 hunks).github/workflows/nodejs.yml(1 hunks).travis.yml(0 hunks)README.md(1 hunks)appveyor.yml(0 hunks)exit.js(2 hunks)index.js(1 hunks)package.json(1 hunks)test/fixtures/before-exit.js(1 hunks)test/index.test.js(4 hunks)
💤 Files with no reviewable changes (3)
- .travis.yml
- appveyor.yml
- .autod.conf.js
🧰 Additional context used
🪛 Biome (1.9.4)
exit.js
[error] 13-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
exit.js (1)
Line range hint 17-27: Verify timeout handling in the codebase
The timeout implementation looks correct, but we should verify its usage across the codebase.
Additionally, consider adding a comment documenting the timeout behavior.
return function exitFunction(code) {
+ // Enforces timeout on beforeExit execution
+ // If timeout occurs, process exits with original code
pt.timeout(handler(), timeout)✅ Verification successful
Let me gather more information about the timeout implementation to ensure it's used consistently.
Let me check the implementation of the pt module to understand the timeout handling better.
Timeout handling implementation is correctly used and consistent
The verification shows that:
- Timeout is properly configured through
GRACEFUL_TIMEOUTenv var or options with a default of 5000ms - The implementation uses the
promise-timeoutnpm package consistently - Timeout is only used in the exit handler, which is the intended behavior
- The error handling properly logs failures and maintains the original exit code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for timeout-related configurations and usage
rg -A 2 "GRACEFUL_TIMEOUT|getExitFunction.*timeout"
Length of output: 792
Script:
#!/bin/bash
# Check the actual timeout implementation and its usage
ast-grep --pattern 'pt.timeout($$$)'
# Also check for any other timeout-related implementations
rg -A 3 "\.timeout\("
Length of output: 278
Script:
#!/bin/bash
# Find the pt module implementation
fd "pt.js|promise-timeout.js" --type f
# Check imports related to timeout functionality
rg -A 2 "require.*pt|require.*timeout"
Length of output: 225
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
[error] 13-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
package.json (2)
32-35: Verify egg-bin major version upgrade impact
The major version upgrade of egg-bin from 4.x to 6.x might introduce breaking changes.
26-27: Verify promise-timeout version compatibility and security
The new promise-timeout dependency should be verified for compatibility with Node.js >=14 and potential security issues.
✅ Verification successful
Let me gather more information about the usage context of promise-timeout in the codebase.
Let me check how the promise-timeout is actually used in the exit.js file to understand the implementation context better.
promise-timeout dependency is safe to use
The package is used appropriately in exit.js for timing out promise-based exit handlers, has no known vulnerabilities, and is still maintained (last updated 2022). While it doesn't explicitly specify Node.js version compatibility, the implementation uses standard Promise APIs that are fully supported in Node.js 14+. The simple implementation and focused use case in exit.js suggests minimal risk.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check promise-timeout package
curl -s https://registry.npmjs.org/promise-timeout | jq '.versions["1.3.0"].engines'
# Check for known vulnerabilities
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "promise-timeout") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
}
}
}'
Length of output: 385
Script:
#!/bin/bash
# Find files using promise-timeout
rg -l "promise-timeout"
# Check package.json for Node.js version requirement
cat package.json | jq '.engines'
# Look for actual usage patterns
rg "require.*promise-timeout|import.*promise-timeout" -A 2
Length of output: 287
Script:
#!/bin/bash
# Check the implementation in exit.js
cat exit.js
# Check the package's last publish date and maintenance status
curl -s https://registry.npmjs.org/promise-timeout | jq '{modified: ."time"."modified", created: ."time"."created", maintainers: .maintainers}'
Length of output: 1125
test/fixtures/before-exit.js (1)
25-32: LGTM: Well-structured test case for timeout functionality
The test case effectively validates the timeout feature by:
- Setting up a long-running operation (5000ms sleep)
- Matching the default timeout duration from index.js
- Including appropriate error handling
README.md (1)
5-10: LGTM: Updated badges align with modern CI transition
Badge updates appropriately reflect:
- Transition to GitHub Actions
- Removal of legacy CI systems
- Consistent styling using shield.io
index.js (1)
35-35: LGTM: Clean integration of timeout parameter
The timeout parameter is properly integrated into the getExitFunction call, maintaining the existing function structure while adding the new capability.
test/index.test.js (2)
8-8: Verify fkill dependency version.
The fkill dependency has been added but its version is not pinned in this file. Please ensure the version is specified in package.json to prevent potential breaking changes.
✅ Verification successful
The fkill dependency is properly pinned in devDependencies
The fkill package is correctly specified as a dev dependency with version ^7.2.1 in package.json, which is appropriate since it's only used in test files. The caret range ^7.2.1 allows for compatible updates while preventing breaking changes, following npm best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if fkill version is pinned in package.json
cat package.json | jq '.dependencies.fkill, .devDependencies.fkill'
Length of output: 83
Line range hint 1-214: Verify Node.js version compatibility.
The test file uses modern async/await syntax which aligns with dropping Node 6.x support. Let's verify the Node.js version requirements in package.json.
✅ Verification successful
Node.js version compatibility verified
The test file's use of modern async/await syntax is well-aligned with the project's Node.js support. The CI workflow confirms testing on Node.js versions 14, 16, 18, 20, and 22, which are all modern versions that fully support async/await. The absence of Node.js 6.x in CI and the modern syntax usage correctly reflects the dropping of legacy Node.js support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Node.js version requirements
cat package.json | jq '.engines.node'
Length of output: 42
Script:
#!/bin/bash
# Let's check if Node.js version requirements are specified in dependencies or devDependencies
cat package.json | jq '{dependencies, devDependencies}'
Length of output: 399
Script:
#!/bin/bash
# Let's check the README.md and other documentation files for Node.js version requirements
fd -e md | xargs cat
Length of output: 2117
Script:
#!/bin/bash
# Let's check the CI workflow file to see which Node.js versions are being tested
cat .github/workflows/nodejs.yml
Length of output: 380
| const handler = once(() => { | ||
| return new Promise(resolve => { | ||
| resolve(beforeExit && beforeExit()); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling rejection in the Promise constructor
The Promise constructor should handle potential rejections from beforeExit().
const handler = once(() => {
- return new Promise(resolve => {
+ return new Promise((resolve, reject) => {
resolve(beforeExit && beforeExit());
- });
+ }).catch(reject);
});Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| uses: node-modules/github-actions/.github/workflows/node-test.yml@master | ||
| with: | ||
| os: 'ubuntu-latest, macos-latest' | ||
| version: '14, 16, 18, 20, 22' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify the external workflow template
The external workflow template should be pinned to a specific version for stability.
- uses: node-modules/github-actions/.github/workflows/node-test.yml@master
+ uses: node-modules/github-actions/.github/workflows/[email protected] # Replace with actual versionAlso, consider adding Windows to the OS matrix for broader coverage.
- os: 'ubuntu-latest, macos-latest'
+ os: 'ubuntu-latest, macos-latest, windows-latest'Committable suggestion skipped: line range outside the PR's diff.
| printLogLevels.warn = false; | ||
| } | ||
| const label = options.label || `graceful-process#${process.pid}`; | ||
| const timeout = options.timeout || parseInt(process.env.GRACEFUL_TIMEOUT) || 5000; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add radix parameter to parseInt and validate timeout value
While the timeout implementation is good, consider these improvements:
- Add radix parameter to parseInt
- Validate for positive timeout values
- const timeout = options.timeout || parseInt(process.env.GRACEFUL_TIMEOUT) || 5000;
+ const timeout = options.timeout || parseInt(process.env.GRACEFUL_TIMEOUT, 10) || 5000;
+ if (timeout <= 0) {
+ throw new Error('timeout must be a positive number');
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const timeout = options.timeout || parseInt(process.env.GRACEFUL_TIMEOUT) || 5000; | |
| const timeout = options.timeout || parseInt(process.env.GRACEFUL_TIMEOUT, 10) || 5000; | |
| if (timeout <= 0) { | |
| throw new Error('timeout must be a positive number'); | |
| } |
| it('should exit when function error', async () => { | ||
| mm(process.env, 'MODE', 'timeout'); | ||
| mm(process.env, 'GRACEFUL_TIMEOUT', 1000); | ||
| const startFile = path.join(fixtures, 'before-exit.js'); | ||
| const child = coffee.fork(startFile) | ||
| .debug(); | ||
| await sleep(waitStart); | ||
| const result = await urllib.request('http://127.0.0.1:8000/'); | ||
| assert(result.status === 200); | ||
|
|
||
| await killChild(child); | ||
| await sleep(5000); | ||
| child.expect('stderr', /beforeExit fail, error: Timeout/); | ||
| child.expect('stdout', /exit with code:0/); | ||
| }); |
There was a problem hiding this comment.
Fix duplicate test name and improve timeout configuration.
- The test name "should exit when function error" is duplicated with the previous test
- The timeout value could be extracted to a constant for better maintainability
- it('should exit when function error', async () => {
+ it('should exit when timeout occurs', async () => {
mm(process.env, 'MODE', 'timeout');
- mm(process.env, 'GRACEFUL_TIMEOUT', 1000);
+ const GRACEFUL_TIMEOUT = 1000;
+ mm(process.env, 'GRACEFUL_TIMEOUT', GRACEFUL_TIMEOUT);
// ... test code ...
});📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should exit when function error', async () => { | |
| mm(process.env, 'MODE', 'timeout'); | |
| mm(process.env, 'GRACEFUL_TIMEOUT', 1000); | |
| const startFile = path.join(fixtures, 'before-exit.js'); | |
| const child = coffee.fork(startFile) | |
| .debug(); | |
| await sleep(waitStart); | |
| const result = await urllib.request('http://127.0.0.1:8000/'); | |
| assert(result.status === 200); | |
| await killChild(child); | |
| await sleep(5000); | |
| child.expect('stderr', /beforeExit fail, error: Timeout/); | |
| child.expect('stdout', /exit with code:0/); | |
| }); | |
| it('should exit when timeout occurs', async () => { | |
| mm(process.env, 'MODE', 'timeout'); | |
| const GRACEFUL_TIMEOUT = 1000; | |
| mm(process.env, 'GRACEFUL_TIMEOUT', GRACEFUL_TIMEOUT); | |
| const startFile = path.join(fixtures, 'before-exit.js'); | |
| const child = coffee.fork(startFile) | |
| .debug(); | |
| await sleep(waitStart); | |
| const result = await urllib.request('http://127.0.0.1:8000/'); | |
| assert(result.status === 200); | |
| await killChild(child); | |
| await sleep(5000); | |
| child.expect('stderr', /beforeExit fail, error: Timeout/); | |
| child.expect('stdout', /exit with code:0/); | |
| }); |
fengmk2
changed the title
[skip ci] ## [2.0.0](v1.2.0...v2.0.0) (2024-12-15) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced new GitHub Actions workflows for publishing and release processes. - Added TypeScript configuration for stricter type-checking and modern module support. - New exit handler functionality for graceful application termination. - **Documentation** - Updated README.md for clarity on usage and installation instructions. - **Bug Fixes** - Improved error handling and logging in the exit process. - **Chores** - Updated `.gitignore` and `package.json` to reflect new structure and dependencies. - Modified CI pipeline to use updated Node.js versions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#8](#8)) ([a31a80e](a31a80e)) * support timeout && drop node 14 support ([#6](#6)) ([8551bae](8551bae))
egg-cluster 里面的 beforeExit 是 agent.close，有可能存在某些插件的监听了 lifecycle 的 beforeClose 然后一直没返回的情况
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests