Skip to content

Add promise return for getSocket #179

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

Merged
merged 1 commit into from
Apr 2, 2025
Merged

Add promise return for getSocket #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 additions & 0 deletions lib/portfinder.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@ interface PortFinderOptions {
stopPort?: number;
}

type SocketfinderCallback = (err: Error, socket: string) => void;

interface SocketFinderOptions {
/**
* Mode to use when creating folder for socket if it doesn't exist
*/
mod?: number;
/**
* Path to the socket file to create
* (defaults to `${exports.basePath}.sock` if not provided)
*/
path?: string;
}

/**
* The lowest port to begin any port search from.
*/
Expand Down Expand Up @@ -81,3 +95,9 @@ export function getPorts(count: number, options: PortFinderOptions, callback: (e
* Responds a promise that resolves to an array of unbound ports on the current machine.
*/
export function getPortsPromise(count: number, options?: PortFinderOptions): Promise<Array<number>>;

export function getSocket(options: SocketFinderOptions): Promise<string>;
export function getSocket(callback: SocketfinderCallback): void;
export function getSocket(options: SocketFinderOptions, callback: SocketfinderCallback): void;

export function getSocketPromise(options?: SocketFinderOptions): Promise<string>;
51 changes: 35 additions & 16 deletions lib/portfinder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ exports.getPort = function (options, callback) {
});
});
} else {
return internals.getPort(options, callback);
internals.getPort(options, callback);
}
};

Expand Down Expand Up @@ -286,7 +286,7 @@ exports.getPorts = function (count, options, callback) {
});
});
} else {
return internals.getPorts(count, options, callback);
internals.getPorts(count, options, callback);
Copy link
Contributor Author

@MasterOdin MasterOdin Mar 15, 2025

Choose a reason for hiding this comment

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

This and the change to internals.getPort above are no-ops in terms of what they do (as the functions don't return anything), but I feel this is more "semantically correct" in that we don't expect this function to return anything if not a promise.

}
};

Expand All @@ -298,19 +298,7 @@ exports.getPorts = function (count, options, callback) {
//
exports.getPortsPromise = exports.getPorts;

//
// ### function getSocket (options, callback)
// #### @options {Object} Settings to use when finding the necessary port
// #### @callback {function} Continuation to respond to when complete.
// Responds with a unbound socket using the specified directory and base
// name on the current machine.
//
exports.getSocket = function (options, callback) {
if (!callback) {
callback = options;
options = {};
}

internals.getSocket = function (options, callback) {
options.mod = options.mod || parseInt(755, 8);
options.path = options.path || exports.basePath + '.sock';

Expand All @@ -337,7 +325,7 @@ exports.getSocket = function (options, callback) {
// next socket.
//
options.path = exports.nextSocket(options.path);
exports.getSocket(options, callback);
internals.getSocket(options, callback);
}
});
}
Expand Down Expand Up @@ -386,6 +374,37 @@ exports.getSocket = function (options, callback) {
: checkAndTestSocket();
};

//
// ### function getSocket (options, callback)
// #### @options {Object} Settings to use when finding the necessary port
// #### @callback {function} Continuation to respond to when complete.
// Responds with a unbound socket using the specified directory and base
// name on the current machine.
//
exports.getSocket = function (options, callback) {
if (typeof options === 'function') {
callback = options;
options = {};
}

options = options || {};

if (!callback) {
return new Promise(function (resolve, reject) {
internals.getSocket(options, function (err, socketPath) {
if (err) {
return reject(err);
}
resolve(socketPath);
});
});
} else {
internals.getSocket(options, callback);
}
}

exports.getSocketPromise = exports.getSocket;

//
// ### function nextPort (port)
// #### @port {Number} Port to increment from.
Expand Down
164 changes: 114 additions & 50 deletions test/port-finder-socket.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ function createServers (callback) {
function (next) {
const server = net.createServer(function () { }),
name = base === 0 ? 'test.sock' : 'test' + base + '.sock';
let sock = path.join(socketDir, name);
const socket = path.join(socketDir, name);
let sock = socket;
Copy link
Member

@eriktrom eriktrom Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can we just have 1 variable socket? Or am I missing something?

Copy link
Contributor Author

@MasterOdin MasterOdin Mar 21, 2025

Choose a reason for hiding this comment

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

If the platform is windows, we append the pipe stuff to the front of sock to be passed to server.listen. However, we create the file without the pipe stuff, so that's why I've used the two separate variables, socket to track the filename as generated, and then sock to be modified with the pipe stuff.

Copy link
Member

@eriktrom eriktrom Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

sounds good


// shamelessly stolen from foreverjs,
// https://github.com/foreverjs/forever/blob/6d143609dd3712a1cf1bc515d24ac6b9d32b2588/lib/forever/worker.js#L141-L154
Expand All @@ -46,22 +47,19 @@ function createServers (callback) {

server.listen(sock, next);
base++;
servers.push(server);
servers.push([server, socket]);
}, callback);
}

function stopServers(callback, index) {
if (index < servers.length) {
servers[index].close(function (err) {
if (err) {
callback(err, false);
} else {
stopServers(callback, index + 1);
function stopServers(callback) {
_async.each(servers, function ([server, socket], next) {
server.close(function () {
if (process.platform === 'win32' && fs.existsSync(socket)) {
fs.unlinkSync(socket);
}
next();
});
} else {
callback(null, true);
}
}, callback);
}

function cleanup(callback) {
Expand All @@ -74,7 +72,7 @@ function cleanup(callback) {
if (fs.existsSync(badDir)) {
fs.rmdirSync(badDir);
}
stopServers(callback, 0);
stopServers(callback);
}

describe('portfinder', function () {
Expand All @@ -88,60 +86,126 @@ describe('portfinder', function () {

describe('with 5 existing servers', function () {
beforeAll(function (done) {
createServers(function () {
portfinder.getSocket({
path: path.join(badDir, 'test.sock'),
}, function () {
done();
});
Comment on lines -92 to -96
Copy link
Contributor Author

@MasterOdin MasterOdin Mar 15, 2025

Choose a reason for hiding this comment

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

Calling getSocket here was probably not meaningfully necessary to waiting for createServers to finish, so I removed it in favor of just calling done.

});
createServers(done);
});

afterAll(function (done) {
stopServers(done);
});

test('the getSocket() method should respond with the first free socket (test5.sock)', function (done) {
portfinder.getSocket({
path: path.join(socketDir, 'test.sock'),
}, function (err, socket) {
expect(err).toBeNull();
expect(socket).toEqual(path.join(socketDir, 'test5.sock'));
done();
describe.each([
['getSocket()', false, portfinder.getSocket],
['getSocket()', true, portfinder.getSocket],
['getSocketPromise()', true, portfinder.getSocketPromise],
])(`the %s method (promise: %p)`, function (name, isPromise, method) {
test('should respond with the first free socket (test5.sock)', function (done) {
if (isPromise) {
method({
path: path.join(socketDir, 'test.sock')
})
.then(function (socket) {
expect(socket).toEqual(path.join(socketDir, 'test5.sock'));
done();
})
.catch(function (err) {
done(err);
});
} else {
method({
path: path.join(socketDir, 'test.sock'),
}, function (err, socket) {
if (err) {
done(err);
return;
}
expect(err).toBeNull();
expect(socket).toEqual(path.join(socketDir, 'test5.sock'));
done();
});
}
});
});
});

describe('with no existing servers', function () {
describe('the getSocket() method', function () {
describe.each([
['getSocket()', false, portfinder.getSocket],
['getSocket()', true, portfinder.getSocket],
['getSocketPromise()', true, portfinder.getSocketPromise],
])(`the %s method (promise: %p)`, function (name, isPromise, method) {
test("with a directory that doesn't exist should respond with the first free socket (test.sock)", function (done) {
portfinder.getSocket({
path: path.join(badDir, 'test.sock'),
}, function (err, socket) {
expect(err).toBeNull();
expect(socket).toEqual(path.join(badDir, 'test.sock'));
done();
});
if (isPromise) {
method({
path: path.join(badDir, 'test.sock'),
})
.then(function (socket) {
expect(socket).toEqual(path.join(badDir, 'test.sock'));
done();
})
.catch(function (err) {
done(err);
});
} else {
method({
path: path.join(badDir, 'test.sock'),
}, function (err, socket) {
if (err) {
done(err);
return;
}
expect(err).toBeNull();
expect(socket).toEqual(path.join(badDir, 'test.sock'));
done();
});
}
});

test("with a nested directory that doesn't exist should respond with the first free socket (test.sock)", function (done) {
portfinder.getSocket({
path: path.join(badDir, 'deeply', 'nested', 'test.sock'),
}, function (err, socket) {
expect(err).toBeNull();
expect(socket).toEqual(path.join(badDir, 'deeply', 'nested', 'test.sock'));
done();
});
if (isPromise) {
method({
path: path.join(badDir, 'deeply', 'nested', 'test.sock'),
})
.then(function (socket) {
expect(socket).toEqual(path.join(badDir, 'deeply', 'nested', 'test.sock'));
done();
})
.catch(function (err) {
done(err);
});
} else {
method({
path: path.join(badDir, 'deeply', 'nested', 'test.sock'),
}, function (err, socket) {
expect(err).toBeNull();
expect(socket).toEqual(path.join(badDir, 'deeply', 'nested', 'test.sock'));
done();
});
}
});

test('with a directory that exists should respond with the first free socket (test.sock)', function (done) {
portfinder.getSocket({
path: path.join(socketDir, 'exists.sock'),
}, function (err, socket) {
expect(err).toBeNull();
expect(socket).toEqual(path.join(socketDir, 'exists.sock'));
done();
});
// We don't use `test.sock` here due to some race condition on Windows in freeing the `test.sock` file
// when we close the servers.
test('with a directory that exists should respond with the first free socket (foo.sock)', function (done) {
Comment on lines +186 to +188
Copy link
Contributor Author

@MasterOdin MasterOdin Mar 15, 2025

Choose a reason for hiding this comment

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

The test was mentioning test.sock, but the code was using exists.sock. I'm guessing this was due to an issue on windows where the socket files were not being cleared as part of cleanup. While this was fixed, I think there's still some weird race condition where calling this function too quickly will still fail on Windows if using test.sock.

As such, I went with calling it foo.sock to avoid the name collision and that I renamed it as I didn't like calling it exists.sock as that felt like it was implying the socket file already existed.

if (isPromise) {
method({
path: path.join(socketDir, 'foo.sock'),
})
.then(function (socket) {
expect(socket).toEqual(path.join(socketDir, 'foo.sock'));
done();
})
.catch(function (err) {
done(err);
});
} else {
method({
path: path.join(socketDir, 'foo.sock'),
}, function (err, socket) {
expect(err).toBeNull();
expect(socket).toEqual(path.join(socketDir, 'foo.sock'));
done();
});
}
});
});
});
Expand Down