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.

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

Feature/port availability enhancements #5157

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions lib/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,21 @@ class Server {
searchParams.set("password", webSocketURL.password);
}

// Initialize an array to keep track of applied middleware
const appliedMiddleware = [];

// Function to apply middleware only once
function applyMiddlewareOnce(app, middleware) {
// Check if the middleware has already been applied
if (!appliedMiddleware.includes(middleware)) {
// Apply the middleware
app.use(middleware);

// Add the middleware to the applied middleware array
appliedMiddleware.push(middleware);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Why we need it here?

Copy link
Author

Choose a reason for hiding this comment

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

@alexander-akait This code is used to ensure that middleware is applied only once to an Express.js application. Middleware functions are often applied to handle tasks like authentication, logging, or error handling. By keeping track of applied middleware in an array and checking if a middleware has already been applied before applying it again, this code prevents duplicate application of middleware, which can cause unexpected behavior or performance issues in an Express.js application.

Copy link
Member

Choose a reason for hiding this comment

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

Please avoid extra code from PR and put only required for your improvement/fix

Copy link
Author

Choose a reason for hiding this comment

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

Okay i will take care of it for further contribution


/** @type {string} */
let hostname;

Expand Down
103 changes: 39 additions & 64 deletions lib/getPort.js
Original file line number Diff line number Diff line change
@@ -1,27 +1,18 @@
"use strict";

/*
* Based on the packages get-port https://www.npmjs.com/package/get-port
* and portfinder https://www.npmjs.com/package/portfinder
* The code structure is similar to get-port, but it searches
* ports deterministically like portfinder
*/
const net = require("net");
const os = require("os");

const minPort = 1024;
const maxPort = 65_535;
const maxPort = 65535;

/**
* @return {Set<string|undefined>}
* Get local host addresses including both IPv4 and IPv6.
* @return {Set<string|undefined>} Set of local host addresses.
*/
const getLocalHosts = () => {
const interfaces = os.networkInterfaces();

// Add undefined value for createServer function to use default host,
// and default IPv4 host in case createServer defaults to IPv6.
// eslint-disable-next-line no-undefined
const results = new Set([undefined, "0.0.0.0"]);
const results = new Set([undefined, "0.0.0.0", "::"]); // Default hosts for IPv4 and IPv6

for (const _interface of Object.values(interfaces)) {
if (_interface) {
Expand All @@ -35,99 +26,83 @@ const getLocalHosts = () => {
};

/**
* @param {number} basePort
* @param {string | undefined} host
* @return {Promise<number>}
* Check if a port is available on a specific host.
* @param {number} basePort - The port to check for availability.
* @param {string|undefined} host - The host address to check on.
* @return {Promise<number>} A promise resolving to the available port.
*/
const checkAvailablePort = (basePort, host) =>
new Promise((resolve, reject) => {
const checkAvailablePort = (basePort, host) => {
return new Promise((resolve, reject) => {
const server = net.createServer();
server.unref();
server.on("error", reject);
server.on("error", (err) => {
server.close();
reject(err);
});

server.listen(basePort, host, () => {
// Next line should return AddressInfo because we're calling it after listen() and before close()
const { port } = /** @type {import("net").AddressInfo} */ (
server.address()
);
const { port } = server.address();
server.close(() => {
resolve(port);
});
});
});
};

/**
* @param {number} port
* @param {Set<string|undefined>} hosts
* @return {Promise<number>}
* Get an available port within a range on any of the specified hosts.
* @param {number} basePort - The base port to start searching from.
* @param {string|undefined} host - The host address to search on.
* @return {Promise<number>} A promise resolving to the available port.
*/
const getAvailablePort = async (port, hosts) => {
/**
* Errors that mean that host is not available.
* @type {Set<string | undefined>}
*/
const getAvailablePort = async (basePort, host) => {
const localhosts = getLocalHosts();
let hosts;
if (host && !localhosts.has(host)) {
hosts = new Set([host]);
} else {
hosts = localhosts;
}

const nonExistentInterfaceErrors = new Set(["EADDRNOTAVAIL", "EINVAL"]);
/* Check if the post is available on every local host name */
for (const host of hosts) {
try {
await checkAvailablePort(port, host); // eslint-disable-line no-await-in-loop
await checkAvailablePort(basePort, host);
} catch (error) {
/* We throw an error only if the interface exists */
if (
!nonExistentInterfaceErrors.has(
/** @type {NodeJS.ErrnoException} */ (error).code,
)
) {
if (!(error instanceof Error) || !nonExistentInterfaceErrors.has(error.code)) {
throw error;
}
}
}

return port;
return basePort;
};

/**
* @param {number} basePort
* @param {string=} host
* @return {Promise<number>}
* Get an available port within a range on any of the specified hosts.
* @param {number} basePort - The base port to start searching from.
* @param {string=} host - The host address to search on.
* @return {Promise<number>} A promise resolving to the available port.
*/
async function getPorts(basePort, host) {
if (basePort < minPort || basePort > maxPort) {
throw new Error(`Port number must lie between ${minPort} and ${maxPort}`);
}

let port = basePort;
const localhosts = getLocalHosts();
let hosts;
if (host && !localhosts.has(host)) {
hosts = new Set([host]);
} else {
/* If the host is equivalent to localhost
we need to check every equivalent host
else the port might falsely appear as available
on some operating systems */
hosts = localhosts;
}
/** @type {Set<string | undefined>} */
const portUnavailableErrors = new Set(["EADDRINUSE", "EACCES"]);
while (port <= maxPort) {
try {
const availablePort = await getAvailablePort(port, hosts); // eslint-disable-line no-await-in-loop
const availablePort = await getAvailablePort(port, host);
return availablePort;
} catch (error) {
/* Try next port if port is busy; throw for any other error */
if (
!portUnavailableErrors.has(
/** @type {NodeJS.ErrnoException} */ (error).code,
)
) {
if (!(error instanceof Error) || !["EADDRINUSE", "EACCES"].includes(error.code)) {
throw error;
}
port += 1;
}
}

throw new Error("No available ports found");
throw new Error(`No available ports found in the range ${basePort} to ${maxPort}`);
}

module.exports = getPorts;
39 changes: 39 additions & 0 deletions test/server/open-option.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -959,3 +959,42 @@ describe('"open" option', () => {
loggerWarnSpy.mockRestore();
});
});

// applyMiddlewareOnce-test

const { applyMiddlewareOnce } = require('../../lib/Server');

describe('applyMiddlewareOnce', () => {
let app;

beforeEach(() => {
// Mock Express app
app = {
use: jest.fn()
};
});

afterEach(() => {
jest.clearAllMocks();
});

it('should apply middleware only once', () => {
const middlewareFunction = jest.fn();
applyMiddlewareOnce(app, middlewareFunction);
applyMiddlewareOnce(app, middlewareFunction);

expect(app.use).toHaveBeenCalledTimes(1);
expect(app.use).toHaveBeenCalledWith(middlewareFunction);
});

it('should apply different middleware functions separately', () => {
const middlewareFunction1 = jest.fn();
const middlewareFunction2 = jest.fn();
applyMiddlewareOnce(app, middlewareFunction1);
applyMiddlewareOnce(app, middlewareFunction2);

expect(app.use).toHaveBeenCalledTimes(2);
expect(app.use).toHaveBeenCalledWith(middlewareFunction1);
expect(app.use).toHaveBeenCalledWith(middlewareFunction2);
});
});
40 changes: 40 additions & 0 deletions test/server/proxy-option.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1035,3 +1035,43 @@ describe("proxy option", () => {
});
});
});


// applyMiddlewareOnce-test

const { applyMiddlewareOnce } = require('../../lib/Server');

describe('applyMiddlewareOnce', () => {
let app;

beforeEach(() => {
// Mock Express app
app = {
use: jest.fn()
};
});

afterEach(() => {
jest.clearAllMocks();
});

it('should apply middleware only once', () => {
const middlewareFunction = jest.fn();
applyMiddlewareOnce(app, middlewareFunction);
applyMiddlewareOnce(app, middlewareFunction);

expect(app.use).toHaveBeenCalledTimes(1);
expect(app.use).toHaveBeenCalledWith(middlewareFunction);
});

it('should apply different middleware functions separately', () => {
const middlewareFunction1 = jest.fn();
const middlewareFunction2 = jest.fn();
applyMiddlewareOnce(app, middlewareFunction1);
applyMiddlewareOnce(app, middlewareFunction2);

expect(app.use).toHaveBeenCalledTimes(2);
expect(app.use).toHaveBeenCalledWith(middlewareFunction1);
expect(app.use).toHaveBeenCalledWith(middlewareFunction2);
});
});