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.

Sign up for GitHub

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: refactor with async/await instead of callback #59

Merged
merged 3 commits into from
Dec 10, 2024
Merged

feat: refactor with async/await instead of callback #59

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f6ce4d8
feat: refactor with async/await instead of callback
fengmk2 Dec 10, 2024
687a374
f
fengmk2 Dec 10, 2024
81bb5c1
f
fengmk2 Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/bin/detect-port.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import path from 'node:path';
import { readFileSync } from 'node:fs';
import detectPort from '../detect-port.js';
import { detectPort } from '../detect-port.js';

const pkgFile = path.join(__dirname, '../../../package.json');
const pkg = JSON.parse(readFileSync(pkgFile, 'utf-8'));
Expand Down
171 changes: 91 additions & 80 deletions src/detect-port.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,26 @@

const debug = debuglog('detect-port');

type DetectPortCallback = (err: Error | null, port?: number) => void;
export type DetectPortCallback = (err: Error | null, port?: number) => void;

interface PortConfig {
export interface PortConfig {
port?: number | string;
hostname?: string | undefined;
callback?: DetectPortCallback;
}

export default function detectPort(port?: number | PortConfig | string): Promise<number>;
export default function detectPort(callback: DetectPortCallback): void;
export default function detectPort(port: number | PortConfig | string | undefined, callback: DetectPortCallback): void;
export default function detectPort(port?: number | string | PortConfig | DetectPortCallback, callback?: DetectPortCallback) {
export class IPAddressNotAvailableError extends Error {
constructor(options?: ErrorOptions) {
super('The IP address is not available on this machine', options);
this.name = this.constructor.name;
Error.captureStackTrace(this, this.constructor);
}

Check warning on line 20 in src/detect-port.ts

View check run for this annotation

Codecov / codecov/patch

src/detect-port.ts#L17-L20 

Added lines #L17 - L20 were not covered by tests
}

export function detectPort(port?: number | PortConfig | string): Promise<number>;
export function detectPort(callback: DetectPortCallback): void;
export function detectPort(port: number | PortConfig | string | undefined, callback: DetectPortCallback): void;
export function detectPort(port?: number | string | PortConfig | DetectPortCallback, callback?: DetectPortCallback) {
let hostname: string | undefined = '';

if (port && typeof port === 'object') {
Expand All @@ -36,99 +44,102 @@
}
debug('detect free port between [%s, %s)', port, maxPort);
if (typeof callback === 'function') {
return tryListen(port, maxPort, hostname, callback);
return tryListen(port, maxPort, hostname)
.then(port => callback(null, port))
.catch(callback);
}
// promise
return new Promise(resolve => {
tryListen(port as number, maxPort, hostname, (_, realPort) => {
resolve(realPort);
});
});
return tryListen(port as number, maxPort, hostname);
}

function tryListen(port: number, maxPort: number, hostname: string | undefined, callback: DetectPortCallback) {
function handleError() {
port++;
if (port >= maxPort) {
debug('port: %s >= maxPort: %s, give up and use random port', port, maxPort);
port = 0;
maxPort = 0;
}
tryListen(port, maxPort, hostname, callback);
async function handleError(port: number, maxPort: number, hostname?: string) {
if (port >= maxPort) {
debug('port: %s >= maxPort: %s, give up and use random port', port, maxPort);
port = 0;
maxPort = 0;
}
return await tryListen(port, maxPort, hostname);
}

async function tryListen(port: number, maxPort: number, hostname?: string): Promise<number> {
// use user hostname
if (hostname) {
listen(port, hostname, (err, realPort) => {
if (err) {
if ((err as any).code === 'EADDRNOTAVAIL') {
return callback(new Error('The IP address is not available on this machine'));
}
return handleError();
try {
return await listen(port, hostname);
} catch (err: any) {
if (err.code === 'EADDRNOTAVAIL') {
throw new IPAddressNotAvailableError({ cause: err });

Check warning on line 71 in src/detect-port.ts

View check run for this annotation

Codecov / codecov/patch

src/detect-port.ts#L71 

Added line #L71 was not covered by tests
}
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;
}

Check warning on line 84 in src/detect-port.ts

View check run for this annotation

Codecov / codecov/patch

src/detect-port.ts#L83-L84 

Added lines #L83 - L84 were not covered by tests
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);
}

Check warning on line 93 in src/detect-port.ts

View check run for this annotation

Codecov / codecov/patch

src/detect-port.ts#L92-L93 

Added lines #L92 - L93 were not covered by tests

// 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);
}

Check warning on line 100 in src/detect-port.ts

View check run for this annotation

Codecov / codecov/patch

src/detect-port.ts#L99-L100 

Added lines #L99 - L100 were not covered by tests

// 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);
}
}

Check warning on line 111 in src/detect-port.ts

View check run for this annotation

Codecov / codecov/patch

src/detect-port.ts#L106-L111 

Added lines #L106 - L111 were not covered by tests

// 5. check current ip
try {
return await listen(port, ip());
} catch (err) {
return await handleError(++port, maxPort, hostname);

Check warning on line 117 in src/detect-port.ts

View check run for this annotation

Codecov / codecov/patch

src/detect-port.ts#L117 

Added line #L117 was not covered by tests
Comment on lines +68 to +117
Copy link

Choose a reason for hiding this comment

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

🛠️ 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

}
}

function listen(port: number, hostname: string | undefined, callback: DetectPortCallback) {
function listen(port: number, hostname?: string) {
const server = createServer();

server.once('error', err => {
debug('listen %s:%s error: %s', hostname, port, err);
server.close();
return new Promise<number>((resolve, reject) => {
server.once('error', err => {
debug('listen %s:%s error: %s', hostname, port, err);
server.close();

if ((err as any).code === 'ENOTFOUND') {
debug('ignore dns ENOTFOUND error, get free %s:%s', hostname, port);
return callback(null, port);
}
if ((err as any).code === 'ENOTFOUND') {
debug('ignore dns ENOTFOUND error, get free %s:%s', hostname, port);
return resolve(port);
}

Check warning on line 132 in src/detect-port.ts

View check run for this annotation

Codecov / codecov/patch

src/detect-port.ts#L130-L132 

Added lines #L130 - L132 were not covered by tests

return callback(err);
});
return reject(err);
});

debug('try listen %d on %s', port, hostname);
server.listen(port, hostname, () => {
port = (server.address() as AddressInfo).port;
debug('get free %s:%s', hostname, port);
server.close();
return callback(null, port);
debug('try listen %d on %s', port, hostname);
server.listen(port, hostname, () => {
port = (server.address() as AddressInfo).port;
debug('get free %s:%s', hostname, port);
server.close();
return resolve(port);
});
});
}
4 changes: 2 additions & 2 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import detectPort from './detect-port.js';
import { detectPort } from './detect-port.js';

export default detectPort;

export { detectPort };
export * from './detect-port.js';
export * from './wait-port.js';
fengmk2 marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion src/wait-port.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { debuglog } from 'node:util';
import detectPort from './detect-port.js';
import { detectPort } from './detect-port.js';

const debug = debuglog('detect-port:wait-port');

Expand Down
17 changes: 15 additions & 2 deletions test/detect-port.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ import net from 'node:net';
import { strict as assert } from 'node:assert';
import { ip } from 'address';
import mm from 'mm';

import detectPort from '../src/detect-port.js';
import { detectPort } from '../src/detect-port.js';

describe('test/detect-port.test.ts', () => {
afterEach(mm.restore);
Expand Down Expand Up @@ -37,6 +36,13 @@ describe('test/detect-port.test.ts', () => {
server3.listen(28080, '0.0.0.0', cb);
servers.push(server3);

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);
Comment on lines +40 to +45
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Inconsistent error handling patterns found across server setups

The verification reveals inconsistency in error handling:

  • server and server4 have error handlers
  • server2 and server3 lack 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


for (let port = 27000; port < 27010; port++) {
const server = new net.Server();
if (port % 3 === 0) {
Expand Down Expand Up @@ -81,6 +87,13 @@ describe('test/detect-port.test.ts', () => {
assert.equal(realPort, 23001);
});

it('work with listening next port 25001 because 25000 was listened to 127.0.0.1', async () => {
const port = 25000;
const realPort = await detectPort(port);
assert(realPort);
assert.equal(realPort, 25001);
});

it('should listen next port 24001 when localhost is not binding', async () => {
mm(dns, 'lookup', (...args: any[]) => {
mm.restore();
Expand Down
Loading