feat: refactor with async/await instead of callback #59
Conversation
listen 127.0.0.1 and check occupied closes #57
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe pull request introduces modifications to the import statements for the Changes
Assessment against linked issues
Possibly related PRs
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #59 +/- ##
==========================================
- Coverage 91.06% 87.56% -3.51%
==========================================
Files 4 3 -1
Lines 235 201 -34
Branches 60 42 -18
==========================================
- Hits 214 176 -38
- Misses 21 25 +4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
src/detect-port.ts (6)
15-21: Add unit tests forIPAddressNotAvailableErrorThe
IPAddressNotAvailableErrorclass is introduced but is not covered by tests. Adding unit tests for this error will improve test coverage and ensure the error handling works as expected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-20: src/detect-port.ts#L17-L20
Added lines #L17 - L20 were not covered by tests
70-71: Ensure error handling for 'EADDRNOTAVAIL' is testedWhen an error with code
'EADDRNOTAVAIL'is caught, a newIPAddressNotAvailableErroris thrown. This code path is not currently covered by tests. Consider adding a test case that triggers this error to validate the error handling logic.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-71: src/detect-port.ts#L71
Added line #L71 was not covered by tests
83-84: Add tests for random port error casesWhen attempting to listen on a random port (
port === 0), if an error occurs, it is thrown. This scenario is not covered by tests. Include tests that simulate errors when listening on a random port to ensure proper error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 83-84: src/detect-port.ts#L83-L84
Added lines #L83 - L84 were not covered by tests
92-93: Test error handling when listening on '0.0.0.0'The error handling code for failures when listening on
'0.0.0.0'is not covered by tests. Adding tests for this situation will enhance the robustness of the code.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 92-93: src/detect-port.ts#L92-L93
Added lines #L92 - L93 were not covered by tests
115-117: Add tests for listening on current IP addressThe code attempts to listen on the machine's current IP address using
ip(), but errors in this block are not covered by tests. Adding tests for this scenario will improve coverage and verify that the code handles this case properly.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 117-117: src/detect-port.ts#L117
Added line #L117 was not covered by tests
124-143: Specify error types for improved type safetyIn the
listenfunction, errors caught in callbacks are currently untyped (defaulting toany). Consider specifying the error parameter type asNodeJS.ErrnoExceptionto improve type safety and allow access to thecodeproperty without casting.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-132: src/detect-port.ts#L130-L132
Added lines #L130 - L132 were not covered by teststest/detect-port.test.ts (1)
90-95: LGTM! Test case covers the reported issueThe test case properly validates the port detection behavior for 127.0.0.1:25000, addressing the scenario reported in issue #57. The assertions and structure are consistent with other similar tests.
Consider adding an additional test case to verify the behavior when both 25000 and 25001 are occupied, ensuring the implementation correctly finds the next available port in sequence.
it('work with listening next available port when both 25000 and 25001 are occupied on 127.0.0.1', async () => { const server = new net.Server(); await new Promise<void>(resolve => server.listen(25001, '127.0.0.1', () => resolve())); try { const port = 25000; const realPort = await detectPort(port); assert(realPort > 25001); } finally { server.close(); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/bin/detect-port.ts(1 hunks)src/detect-port.ts(2 hunks)src/index.ts(1 hunks)src/wait-port.ts(1 hunks)test/detect-port.test.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/detect-port.ts
[warning] 17-20: src/detect-port.ts#L17-L20
Added lines #L17 - L20 were not covered by tests
[warning] 71-71: src/detect-port.ts#L71
Added line #L71 was not covered by tests
[warning] 83-84: src/detect-port.ts#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 92-93: src/detect-port.ts#L92-L93
Added lines #L92 - L93 were not covered by tests
[warning] 99-100: src/detect-port.ts#L99-L100
Added lines #L99 - L100 were not covered by tests
[warning] 106-111: src/detect-port.ts#L106-L111
Added lines #L106 - L111 were not covered by tests
[warning] 117-117: src/detect-port.ts#L117
Added line #L117 was not covered by tests
[warning] 130-132: src/detect-port.ts#L130-L132
Added lines #L130 - L132 were not covered by tests
🔇 Additional comments (5)
src/detect-port.ts (2)
106-111: Verify logic for 'EADDRNOTAVAIL' when listening on 'localhost'
The special handling of 'EADDRNOTAVAIL' errors when listening on 'localhost' may need to be tested to ensure correctness. Confirm that this behavior is intentional and consider adding tests to cover this logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-111: src/detect-port.ts#L106-L111
Added lines #L106 - L111 were not covered by tests
129-132: Confirm handling of 'ENOTFOUND' errors
When an 'ENOTFOUND' error occurs during server.listen, the error is being ignored and the port is considered available. Please verify that this is the intended behavior and that treating the port as available in this scenario is correct.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 130-132: src/detect-port.ts#L130-L132
Added lines #L130 - L132 were not covered by tests
src/wait-port.ts (1)
2-2: Update usage to match named import of detectPort
The import statement for detectPort has been changed to a named import. Ensure that all usages of detectPort in this file are updated accordingly to prevent any runtime errors.
src/bin/detect-port.ts (1)
5-5: Verify script functionality after import change
The import of detectPort has been updated to a named import. Please verify that the script functions as expected with this change and that all calls to detectPort are properly adjusted.
test/detect-port.test.ts (1)
6-6: LGTM! Import statement modernization
The change from default import to named import aligns with modern ES6+ practices and maintains consistency with the overall refactoring effort.
| return await listen(port, hostname); | ||
| } catch (err: any) { | ||
| if (err.code === 'EADDRNOTAVAIL') { | ||
| throw new IPAddressNotAvailableError({ cause: err }); | ||
| } | ||
| return await handleError(++port, maxPort, hostname); | ||
| } | ||
| } | ||
|
|
||
| callback(null, realPort); | ||
| }); | ||
| } else { | ||
| // 1. check null | ||
| listen(port, void 0, (err, realPort) => { | ||
| // ignore random listening | ||
| if (port === 0) { | ||
| return callback(err, realPort); | ||
| } | ||
| // 1. check null / undefined | ||
| try { | ||
| await listen(port); | ||
| } catch (err) { | ||
| // ignore random listening | ||
| if (port === 0) { | ||
| throw err; | ||
| } | ||
| return await handleError(++port, maxPort, hostname); | ||
| } | ||
|
|
||
| if (err) { | ||
| return handleError(); | ||
| } | ||
| // 2. check 0.0.0.0 | ||
| try { | ||
| await listen(port, '0.0.0.0'); | ||
| } catch (err) { | ||
| return await handleError(++port, maxPort, hostname); | ||
| } | ||
|
|
||
| // 2. check 0.0.0.0 | ||
| listen(port, '0.0.0.0', err => { | ||
| if (err) { | ||
| return handleError(); | ||
| } | ||
|
|
||
| // 3. check localhost | ||
| listen(port, 'localhost', err => { | ||
| // if localhost refer to the ip that is not unkonwn on the machine, you will see the error EADDRNOTAVAIL | ||
| // https://stackoverflow.com/questions/10809740/listen-eaddrnotavail-error-in-node-js | ||
| if (err && (err as any).code !== 'EADDRNOTAVAIL') { | ||
| return handleError(); | ||
| } | ||
|
|
||
| // 4. check current ip | ||
| listen(port, ip(), (err, realPort) => { | ||
| if (err) { | ||
| return handleError(); | ||
| } | ||
|
|
||
| callback(null, realPort); | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
| // 3. check 127.0.0.1 | ||
| try { | ||
| await listen(port, '127.0.0.1'); | ||
| } catch (err) { | ||
| return await handleError(++port, maxPort, hostname); | ||
| } | ||
|
|
||
| // 4. check localhost | ||
| try { | ||
| await listen(port, 'localhost'); | ||
| } catch (err: any) { | ||
| // if localhost refer to the ip that is not unknown on the machine, you will see the error EADDRNOTAVAIL | ||
| // https://stackoverflow.com/questions/10809740/listen-eaddrnotavail-error-in-node-js | ||
| if (err.code !== 'EADDRNOTAVAIL') { | ||
| return await handleError(++port, maxPort, hostname); | ||
| } | ||
| } | ||
|
|
||
| // 5. check current ip | ||
| try { | ||
| return await listen(port, ip()); | ||
| } catch (err) { | ||
| return await handleError(++port, maxPort, hostname); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor recursive calls to iterative loops
The functions tryListen and handleError use recursion to retry listening on incremented ports, which could lead to a stack overflow if many ports are unavailable. Consider refactoring this logic to use iterative loops to enhance reliability and prevent potential stack issues.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-71: src/detect-port.ts#L71
Added line #L71 was not covered by tests
[warning] 83-84: src/detect-port.ts#L83-L84
Added lines #L83 - L84 were not covered by tests
[warning] 92-93: src/detect-port.ts#L92-L93
Added lines #L92 - L93 were not covered by tests
[warning] 99-100: src/detect-port.ts#L99-L100
Added lines #L99 - L100 were not covered by tests
[warning] 106-111: src/detect-port.ts#L106-L111
Added lines #L106 - L111 were not covered by tests
[warning] 117-117: src/detect-port.ts#L117
Added line #L117 was not covered by tests
| const server4 = new net.Server(); | ||
| server4.listen(25000, '127.0.0.1', cb); | ||
| server4.on('error', err => { | ||
| console.error('listen 127.0.0.1 error:', err); | ||
| }); | ||
| servers.push(server4); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent error handling patterns found across server setups
The verification reveals inconsistency in error handling:
serverandserver4have error handlersserver2andserver3lack error handlers, which could lead to unhandled errors
🔗 Analysis chain
LGTM! Server setup addresses the reported issue
The addition of server4 listening on 127.0.0.1:25000 directly addresses the port detection issue reported in #57. The error handling is consistent with existing patterns.
Let's verify that all server setups follow consistent patterns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistency in server setup patterns
# Expected: All server setups should follow similar error handling patterns
rg -A 5 "new net.Server\(\)" test/detect-port.test.ts
Length of output: 918
[skip ci] ## [2.1.0](v2.0.1...v2.1.0) (2024-12-10) ### Features * refactor with async/await instead of callback ([#59](#59)) ([9254c18](9254c18))
listen 127.0.0.1 and check occupied
closes #57
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests