Skip to content

test: Fix common.PIPE to be process unique #14168

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

Merged
merged 1 commit into from
Jul 14, 2017
Merged
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
3 changes: 1 addition & 2 deletions benchmark/http/_chunky_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// test HTTP throughput in fragmented header case
var common = require('../common.js');
var net = require('net');
var test = require('../../test/common');

var bench = common.createBenchmark(main, {
len: [1, 4, 8, 16, 32, 64, 128],
Expand Down Expand Up @@ -56,7 +55,7 @@ function main(conf) {
var mult = 17;
var add = 11;
var count = 0;
var PIPE = test.PIPE;
var PIPE = process.env.PIPE_NAME;
var socket = net.connect(PIPE, function() {
bench.start();
WriteHTTPHeaders(socket, 1, len);
Expand Down
20 changes: 10 additions & 10 deletions benchmark/http/http_server_for_chunky_client.js
Original file line number Diff line number Diff line change
@@ -1,24 +1,22 @@
'use strict';

var assert = require('assert');
var path = require('path');
var http = require('http');
var fs = require('fs');
var fork = require('child_process').fork;
var { fork } = require('child_process');
var common = require('../common.js');
var test = require('../../test/common');
var pep = `${path.dirname(process.argv[1])}/_chunky_http_client.js`;
var PIPE = test.PIPE;
const { PIPE, tmpDir } = require('../../test/common');
process.env.PIPE_NAME = PIPE;

try {
fs.accessSync(test.tmpDir, fs.F_OK);
fs.accessSync(tmpDir, fs.F_OK);
} catch (e) {
fs.mkdirSync(test.tmpDir);
fs.mkdirSync(tmpDir);
}

var server;
try {
fs.unlinkSync(PIPE);
fs.unlinkSync(process.env.PIPE_NAME);
} catch (e) { /* ignore */ }

server = http.createServer(function(req, res) {
Expand All @@ -33,10 +31,12 @@ server = http.createServer(function(req, res) {
server.on('error', function(err) {
throw new Error(`server error: ${err}`);
});

server.listen(PIPE);

var child = fork(pep, process.argv.slice(2));
const child = fork(
`${__dirname}/_chunky_http_client.js`,
process.argv.slice(2)
);
child.on('message', common.sendResult);
child.on('close', function(code) {
server.close();
Expand Down
27 changes: 13 additions & 14 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ Object.defineProperty(exports, 'opensslCli', {get: function() {

Object.defineProperty(exports, 'hasCrypto', {
get: function() {
return process.versions.openssl ? true : false;
return Boolean(process.versions.openssl);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a huge fan of !! outside of minimal code competitions.
Willing to change if anyone has a strong opinion.

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion from me. I think it used to be preferred in hot paths for performance. No idea if that's still the case. Regardless, it's totally sensible to value readable code over performant code in test/common/index.js.

}
});

Expand All @@ -253,22 +253,21 @@ Object.defineProperty(exports, 'hasFipsCrypto', {
}
});

if (exports.isWindows) {
exports.PIPE = '\\\\.\\pipe\\libuv-test';
if (process.env.TEST_THREAD_ID) {
exports.PIPE += `.${process.env.TEST_THREAD_ID}`;
}
} else {
exports.PIPE = `${exports.tmpDir}/test.sock`;
{
Copy link
Member

Choose a reason for hiding this comment

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

Is block scoping really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the const are only used locally and there's a single exported side-effect (exports.PIPE = ...) IMHO it's cleaner.
Similar to the function setupFoo(); setupFoo(); approach in node_bootstap.js
Maybe even V8 will inline the consts ?

Copy link
Member

Choose a reason for hiding this comment

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

I like it, easier to see that the consts aren't used except in exports.PIPE.

const pipePrefix = exports.isWindows ? '\\\\.\\pipe\\' : `${exports.tmpDir}/`;
const pipeName = `node-test.${process.pid}.sock`;
exports.PIPE = pipePrefix + pipeName;
}

const ifaces = os.networkInterfaces();
const re = /lo/;
exports.hasIPv6 = Object.keys(ifaces).some(function(name) {
return re.test(name) && ifaces[name].some(function(info) {
return info.family === 'IPv6';
{
const iFaces = os.networkInterfaces();
const re = /lo/;
exports.hasIPv6 = Object.keys(iFaces).some(function(name) {
return re.test(name) && iFaces[name].some(function(info) {
return info.family === 'IPv6';
});
});
});
}

/*
* Check that when running a test with
Expand Down
39 changes: 0 additions & 39 deletions test/fixtures/listen-on-socket-and-exit.js

This file was deleted.

42 changes: 21 additions & 21 deletions test/parallel/test-cluster-eaccess.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,22 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.

'use strict';
const common = require('../common');

// Test that errors propagated from cluster workers are properly
// received in their master. Creates an EADDRINUSE condition by forking
// a process in child cluster and propagates the error to the master.

const common = require('../common');
const assert = require('assert');
const cluster = require('cluster');
const fork = require('child_process').fork;
const fs = require('fs');
const net = require('net');

if (cluster.isMaster) {
const worker = cluster.fork();
if (cluster.isMaster && process.argv.length !== 3) {
// cluster.isMaster
common.refreshTmpDir();
const PIPE_NAME = common.PIPE;
const worker = cluster.fork({PIPE_NAME});

// makes sure master is able to fork the worker
cluster.on('fork', common.mustCall());
Expand All @@ -43,40 +46,37 @@ if (cluster.isMaster) {
worker.on('message', common.mustCall(function(err) {
// disconnect first, so that we will not leave zombies
worker.disconnect();

console.log(err);
assert.strictEqual('EADDRINUSE', err.code);
}));

process.on('exit', function() {
console.log('master exited');
try {
fs.unlinkSync(common.PIPE);
} catch (e) {
}
});

} else {
common.refreshTmpDir();
const cp = fork(`${common.fixturesDir}/listen-on-socket-and-exit.js`,
{ stdio: 'inherit' });
} else if (process.argv.length !== 3) {
// cluster.worker
const PIPE_NAME = process.env.PIPE_NAME;
const cp = fork(__filename, [PIPE_NAME], { stdio: 'inherit' });

// message from the child indicates it's ready and listening
cp.on('message', common.mustCall(function() {
const server = net.createServer().listen(common.PIPE, function() {
const server = net.createServer().listen(PIPE_NAME, function() {
// message child process so that it can exit
cp.send('end');
// inform master about the unexpected situation
process.send('PIPE should have been in use.');
});

server.on('error', function(err) {
console.log('parent error, ending');
// message to child process tells it to exit
cp.send('end');
// propagate error to parent
process.send(err);
});
}));
} else if (process.argv.length === 3) {
// child process (of cluster.worker)
const PIPE_NAME = process.argv[2];

const server = net.createServer().listen(PIPE_NAME, common.mustCall(() => {
process.send('listening');
}));
process.once('message', common.mustCall(() => server.close()));
} else {
assert.fail('Impossible state');
}