Skip to content

Commit 73efbf3

Browse files
ryancbahanryancbahan
authored
Merge pull request #7177 from Shopify/04-02-remove-get-port-please
Remove get-port-please, use Node net module directly
2 parents 573ad45 + 85adf20 commit 73efbf3

5 files changed

Lines changed: 166 additions & 96 deletions

File tree

‎packages/cli-kit/package.json‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,6 @@
131131
"find-up": "6.3.0",
132132
"form-data": "4.0.4",
133133
"fs-extra": "11.1.0",
134-
"get-port-please": "3.1.2",
135134
"gradient-string": "2.0.2",
136135
"graphql": "16.10.0",
137136
"graphql-request": "6.1.0",

‎packages/cli-kit/src/public/node/tcp-retry.test.ts‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import {AbortError} from './error.js'
2+
import {describe, expect, test, vi, beforeEach} from 'vitest'
3+
import {EventEmitter} from 'events'
4+
5+
vi.mock('./system.js', async (importOriginal) => {
6+
const actual: any = await importOriginal()
7+
return {...actual, sleep: vi.fn()}
8+
})
9+
10+
let callCount = 0
11+
let failCount = 0
12+
13+
vi.mock('net', () => {
14+
return {
15+
createServer: () => {
16+
callCount++
17+
const server = new EventEmitter() as any
18+
server.unref = () => server
19+
server.listen = (_port: number, _host: string, cb?: () => void) => {
20+
if (callCount <= failCount) {
21+
process.nextTick(() => server.emit('error', new Error('mock port allocation failure')))
22+
} else {
23+
server.address = () => ({port: 9999})
24+
process.nextTick(() => cb?.())
25+
}
26+
return server
27+
}
28+
server.close = (cb?: () => void) => {
29+
cb?.()
30+
return server
31+
}
32+
return server
33+
},
34+
}
35+
})
36+
37+
beforeEach(() => {
38+
callCount = 0
39+
failCount = 0
40+
})
41+
42+
describe('getAvailableTCPPort retry behavior', () => {
43+
test('retries and returns port after transient error', async () => {
44+
const {getAvailableTCPPort} = await import('./tcp.js')
45+
const {sleep} = await import('./system.js')
46+
47+
failCount = 1
48+
49+
const got = await getAvailableTCPPort(undefined, {waitTimeInSeconds: 0})
50+
expect(got).toBe(9999)
51+
expect(sleep).toHaveBeenCalledOnce()
52+
})
53+
54+
test('throws AbortError when all retries are exhausted', async () => {
55+
const {getAvailableTCPPort} = await import('./tcp.js')
56+
57+
failCount = 100
58+
59+
await expect(() => getAvailableTCPPort(undefined, {maxTries: 3, waitTimeInSeconds: 0})).rejects.toThrowError(
60+
AbortError,
61+
)
62+
})
63+
})

‎packages/cli-kit/src/public/node/tcp.test.ts‎

Lines changed: 72 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -1,100 +1,93 @@
11
import {getAvailableTCPPort, checkPortAvailability} from './tcp.js'
2-
import * as system from './system.js'
3-
import {AbortError} from './error.js'
4-
import * as port from 'get-port-please'
5-
import {describe, expect, test, vi} from 'vitest'
6-
7-
vi.mock('get-port-please')
8-
9-
const errorMessage = 'Unable to generate random port'
2+
import {describe, expect, test} from 'vitest'
3+
import {createServer} from 'net'
104

115
describe('getAvailableTCPPort', () => {
12-
test('returns random port if the number retries is not exceeded', async () => {
13-
// Given
14-
vi.mocked(port.getRandomPort).mockRejectedValueOnce(new Error(errorMessage))
15-
vi.mocked(port.getRandomPort).mockResolvedValue(5)
16-
const debugError = vi.spyOn(system, 'sleep')
17-
18-
// When
19-
const got = await getAvailableTCPPort(undefined, {waitTimeInSeconds: 0})
20-
21-
// Then
22-
expect(got).toBe(5)
23-
expect(debugError).toHaveBeenCalledOnce()
6+
test('returns a valid port number', async () => {
7+
const port = await getAvailableTCPPort()
8+
expect(port).toBeGreaterThan(0)
9+
expect(port).toBeLessThanOrEqual(65535)
2410
})
2511

26-
test('throws an abort exception with same error message received from third party getRandomPort if the number retries is exceeded', async () => {
27-
// Given
28-
const maxTries = 5
29-
for (let i = 0; i < maxTries; i++) {
30-
vi.mocked(port.getRandomPort).mockRejectedValueOnce(new Error(errorMessage))
31-
}
32-
33-
// When/Then
34-
await expect(() => getAvailableTCPPort(undefined, {waitTimeInSeconds: 0})).rejects.toThrowError(
35-
new AbortError(errorMessage),
36-
)
12+
test('returns the preferred port when it is available', async () => {
13+
const freePort = await getAvailableTCPPort()
14+
const got = await getAvailableTCPPort(freePort)
15+
expect(got).toBe(freePort)
3716
})
3817

39-
test('returns the provided port when it is available', async () => {
40-
// Given
41-
vi.mocked(port.checkPort).mockResolvedValue(666)
42-
43-
// When
44-
const got = await getAvailableTCPPort(666)
45-
46-
// Then
47-
expect(got).toBe(666)
18+
test('returns a different port when the preferred one is in use', async () => {
19+
const server = createServer()
20+
const occupiedPort = await new Promise<number>((resolve) => {
21+
server.listen(0, 'localhost', () => {
22+
const address = server.address()
23+
resolve((address as {port: number}).port)
24+
})
25+
})
26+
27+
try {
28+
const got = await getAvailableTCPPort(occupiedPort)
29+
expect(got).not.toBe(occupiedPort)
30+
expect(got).toBeGreaterThan(0)
31+
} finally {
32+
server.close()
33+
}
4834
})
4935

50-
test('returns a random port when the provided one is not available', async () => {
51-
// Given
52-
vi.mocked(port.checkPort).mockResolvedValue(false)
53-
vi.mocked(port.getRandomPort).mockResolvedValue(5)
54-
55-
// When
56-
const got = await getAvailableTCPPort(666)
57-
58-
// Then
59-
expect(got).toBe(5)
36+
test('returns unique ports across multiple calls', async () => {
37+
const ports = new Set<number>()
38+
for (let i = 0; i < 5; i++) {
39+
// eslint-disable-next-line no-await-in-loop
40+
const port = await getAvailableTCPPort()
41+
ports.add(port)
42+
}
43+
expect(ports.size).toBe(5)
6044
})
6145

62-
test('reserves random ports and does not reuse them', async () => {
63-
vi.mocked(port.checkPort).mockResolvedValue(false)
64-
vi.mocked(port.getRandomPort).mockResolvedValueOnce(55).mockResolvedValueOnce(55).mockResolvedValueOnce(66)
65-
66-
let got = await getAvailableTCPPort(123)
67-
expect(got).toBe(55)
68-
69-
got = await getAvailableTCPPort(123)
70-
expect(got).toBe(66)
46+
test('returns unique ports and all are bindable', async () => {
47+
const ports: number[] = []
48+
for (let i = 0; i < 3; i++) {
49+
// eslint-disable-next-line no-await-in-loop
50+
ports.push(await getAvailableTCPPort())
51+
}
52+
expect(new Set(ports).size).toBe(3)
53+
54+
// Verify all ports are actually bindable
55+
const servers = await Promise.all(
56+
ports.map(
57+
(port) =>
58+
new Promise<ReturnType<typeof createServer>>((resolve, reject) => {
59+
const server = createServer()
60+
server.once('error', reject)
61+
server.listen(port, 'localhost', () => resolve(server))
62+
}),
63+
),
64+
)
65+
// All three bound successfully — clean up
66+
await Promise.all(servers.map((server) => new Promise<void>((resolve) => server.close(() => resolve()))))
7167
})
7268
})
7369

7470
describe('checkPortAvailability', () => {
7571
test('returns true when port is available', async () => {
76-
// Given
77-
const portNumber = 3000
78-
vi.mocked(port.checkPort).mockResolvedValue(portNumber)
79-
80-
// When
81-
const result = await checkPortAvailability(portNumber)
82-
83-
// Then
72+
const freePort = await getAvailableTCPPort()
73+
const result = await checkPortAvailability(freePort)
8474
expect(result).toBe(true)
85-
expect(port.checkPort).toHaveBeenCalledWith(portNumber, 'localhost')
8675
})
8776

88-
test('returns false when port is not available', async () => {
89-
// Given
90-
const portNumber = 3000
91-
vi.mocked(port.checkPort).mockResolvedValue(false)
92-
93-
// When
94-
const result = await checkPortAvailability(portNumber)
95-
96-
// Then
97-
expect(result).toBe(false)
98-
expect(port.checkPort).toHaveBeenCalledWith(portNumber, 'localhost')
77+
test('returns false when port is in use', async () => {
78+
const server = createServer()
79+
const occupiedPort = await new Promise<number>((resolve) => {
80+
server.listen(0, 'localhost', () => {
81+
const address = server.address()
82+
resolve((address as {port: number}).port)
83+
})
84+
})
85+
86+
try {
87+
const result = await checkPortAvailability(occupiedPort)
88+
expect(result).toBe(false)
89+
} finally {
90+
server.close()
91+
}
9992
})
10093
})

‎packages/cli-kit/src/public/node/tcp.ts‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import {sleep} from './system.js'
22
import {AbortError} from './error.js'
33
import {outputDebug, outputContent, outputToken} from './output.js'
4-
import * as port from 'get-port-please'
4+
import {createServer} from 'net'
55

66
interface GetTCPPortOptions {
77
waitTimeInSeconds?: number
@@ -23,14 +23,14 @@ export async function getAvailableTCPPort(preferredPort?: number, options?: GetT
2323
return preferredPort
2424
}
2525
outputDebug(outputContent`Getting a random port...`)
26-
let randomPort = await retryOnError(() => port.getRandomPort(host()), options?.maxTries, options?.waitTimeInSeconds)
26+
let randomPort = await retryOnError(() => getRandomPort(), options?.maxTries, options?.waitTimeInSeconds)
2727

2828
for (let i = 0; i < (options?.maxTries ?? 5); i++) {
2929
if (!obtainedRandomPorts.has(randomPort)) {
3030
break
3131
}
3232
// eslint-disable-next-line no-await-in-loop
33-
randomPort = await retryOnError(() => port.getRandomPort(host()), options?.maxTries, options?.waitTimeInSeconds)
33+
randomPort = await retryOnError(() => getRandomPort(), options?.maxTries, options?.waitTimeInSeconds)
3434
}
3535

3636
outputDebug(outputContent`Random port obtained: ${outputToken.raw(`${randomPort}`)}`)
@@ -45,13 +45,36 @@ export async function getAvailableTCPPort(preferredPort?: number, options?: GetT
4545
* @returns A promise that resolves with a boolean indicating if the port is available.
4646
*/
4747
export async function checkPortAvailability(portNumber: number): Promise<boolean> {
48-
return (await port.checkPort(portNumber, host())) === portNumber
48+
return new Promise((resolve) => {
49+
const server = createServer()
50+
server.unref()
51+
server.once('error', () => resolve(false))
52+
server.listen(portNumber, 'localhost', () => {
53+
server.close(() => resolve(true))
54+
})
55+
})
4956
}
5057

51-
function host(): string | undefined {
52-
// The get-port-please library does not work as expected when HOST env var is defined,
53-
// so explicitly set the host to localhost to avoid conflicts
54-
return 'localhost'
58+
/**
59+
* Gets a random available port by binding to port 0 on localhost.
60+
*
61+
* @returns A promise that resolves with an available port number.
62+
*/
63+
function getRandomPort(): Promise<number> {
64+
return new Promise((resolve, reject) => {
65+
const server = createServer()
66+
server.unref()
67+
server.once('error', reject)
68+
server.listen(0, 'localhost', () => {
69+
const address = server.address()
70+
if (address && typeof address === 'object') {
71+
const assignedPort = address.port
72+
server.close(() => resolve(assignedPort))
73+
} else {
74+
server.close(() => reject(new Error('Unable to determine assigned port')))
75+
}
76+
})
77+
})
5578
}
5679

5780
/**

‎pnpm-lock.yaml‎

Lines changed: 0 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)