From 22dfb2915878e4f337b9bc8af11c82c3aa83a1b2 Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Mon, 24 Jun 2019 16:32:18 -0500 Subject: [PATCH 1/7] refactor(server): move socket handling into server.listen --- bin/webpack-dev-server.js | 34 ----- lib/Server.js | 87 ++++++++++++- test/cli/cli.test.js | 26 ++-- test/helpers/test-server.js | 22 +++- test/server/onListening-option.test.js | 77 +++++++++--- test/server/socket-option.test.js | 164 +++++++++++++++++++++++++ 6 files changed, 340 insertions(+), 70 deletions(-) create mode 100644 test/server/socket-option.test.js diff --git a/bin/webpack-dev-server.js b/bin/webpack-dev-server.js index 7c3456f5d3..9baec711dd 100755 --- a/bin/webpack-dev-server.js +++ b/bin/webpack-dev-server.js @@ -4,8 +4,6 @@ /* eslint-disable no-shadow, no-console */ -const fs = require('fs'); -const net = require('net'); const debug = require('debug')('webpack-dev-server'); const importLocal = require('import-local'); const yargs = require('yargs'); @@ -109,42 +107,10 @@ function startDevServer(config, options) { } if (options.socket) { - server.listeningApp.on('error', (e) => { - if (e.code === 'EADDRINUSE') { - const clientSocket = new net.Socket(); - - clientSocket.on('error', (err) => { - if (err.code === 'ECONNREFUSED') { - // No other server listening on this socket so it can be safely removed - fs.unlinkSync(options.socket); - - server.listen(options.socket, options.host, (error) => { - if (error) { - throw error; - } - }); - } - }); - - clientSocket.connect({ path: options.socket }, () => { - throw new Error('This socket is already used'); - }); - } - }); - server.listen(options.socket, options.host, (err) => { if (err) { throw err; } - - // chmod 666 (rw rw rw) - const READ_WRITE = 438; - - fs.chmod(options.socket, READ_WRITE, (err) => { - if (err) { - throw err; - } - }); }); } else { server.listen(options.port, options.host, (err) => { diff --git a/lib/Server.js b/lib/Server.js index 5d371daae2..b538de9c35 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -6,6 +6,7 @@ func-names */ const fs = require('fs'); +const net = require('net'); const path = require('path'); const tls = require('tls'); const url = require('url'); @@ -34,6 +35,7 @@ const runBonjour = require('./utils/runBonjour'); const routes = require('./utils/routes'); const getSocketServerImplementation = require('./utils/getSocketServerImplementation'); const handleStdin = require('./utils/handleStdin'); +const tryParseInt = require('./utils/tryParseInt'); const schema = require('./options.json'); // Workaround for node ^8.6.0, ^9.0.0 @@ -732,7 +734,7 @@ class Server { listen(port, hostname, fn) { this.hostname = hostname; - return this.listeningApp.listen(port, hostname, (err) => { + const setupCallback = () => { this.createSocketServer(); if (this.options.bonjour) { @@ -740,15 +742,94 @@ class Server { } this.showStatus(); + }; - if (fn) { + // between setupCallback and userCallback should be any other needed handling, + // specifically so that things are done in the right order to prevent + // backwards compatability issues + let userCallbackCalled = false; + const userCallback = (err) => { + if (fn && !userCallbackCalled) { + userCallbackCalled = true; fn.call(this.listeningApp, err); } + }; + const onListeningCallback = () => { if (typeof this.options.onListening === 'function') { this.options.onListening(this); } - }); + }; + + const fullCallback = (err) => { + setupCallback(); + userCallback(err); + onListeningCallback(); + }; + + // try to follow the Node standard in terms of deciding + // whether this is a socket or a port that we will listen on: + // https://github.com/nodejs/node/blob/64219741218aa87e259cf8257596073b8e747f0a/lib/net.js#L196 + if (typeof port === 'string' && tryParseInt(port) === null) { + // in this case the "port" argument is actually a socket path + const socket = port; + // set this so that status helper can identify how the project is being run correctly + this.options.socket = socket; + + const chmodSocket = (done) => { + // chmod 666 (rw rw rw) - octal + const READ_WRITE = 438; + fs.chmod(socket, READ_WRITE, done); + }; + + const startSocket = () => { + this.listeningApp.on('error', (err) => { + userCallback(err); + }); + + this.listeningApp.listen(socket, hostname, (err) => { + if (err) { + fullCallback(err); + } else { + chmodSocket(fullCallback); + } + }); + }; + + fs.access(socket, fs.constants.F_OK, (err) => { + if (err) { + // file does not exist + startSocket(); + } else { + // file exists + + const clientSocket = new net.Socket(); + + clientSocket.on('error', (err) => { + if (err.code === 'ECONNREFUSED' || err.code === 'ENOTSOCK') { + // No other server listening on this socket so it can be safely removed + fs.unlinkSync(socket); + + startSocket(); + } + }); + + clientSocket.connect({ path: socket }, () => { + // if a client socket successfully connects to the given socket path, + // it means that the socket is in use + const err = new Error('This socket is already used'); + clientSocket.destroy(); + // do not call onListening or the setup method, since the server + // cannot start listening on a used socket + userCallback(err); + }); + } + }); + } else { + this.listeningApp.listen(port, hostname, fullCallback); + } + + return this.listeningApp; } close(cb) { diff --git a/test/cli/cli.test.js b/test/cli/cli.test.js index c29bad439f..72a5443ad6 100644 --- a/test/cli/cli.test.js +++ b/test/cli/cli.test.js @@ -9,6 +9,7 @@ const execa = require('execa'); const { unlinkAsync } = require('../helpers/fs'); const testBin = require('../helpers/test-bin'); const timer = require('../helpers/timer'); +const { skipTestOnWindows } = require('../helpers/conditional-test'); const httpsCertificateDirectory = resolve( __dirname, @@ -71,17 +72,24 @@ describe('CLI', () => { expect(stdout.includes('\u001b[39m \u001b[90m｢wds｣\u001b[39m:')).toBe(true); }); - // The Unix socket to listen to (instead of a host). - it('--socket', async () => { - const socketPath = join('.', 'webpack.sock'); - const { exitCode, stdout } = await testBin(`--socket ${socketPath}`); - expect(exitCode).toEqual(0); + describe('Unix socket', () => { + if (skipTestOnWindows('Unix sockets are not supported on Windows')) { + return; + } - if (process.platform !== 'win32') { - expect(stdout.includes(socketPath)).toBe(true); + // The Unix socket to listen to (instead of a host). + it('--socket', async () => { + const socketPath = join('.', 'webpack.sock'); - await unlinkAsync(socketPath); - } + const { exitCode, stdout } = await testBin(`--socket ${socketPath}`); + expect(exitCode).toEqual(0); + + if (process.platform !== 'win32') { + expect(stdout.includes(socketPath)).toBe(true); + + await unlinkAsync(socketPath); + } + }); }); it('without --stdin, with stdin "end" event should time out', async (done) => { diff --git a/test/helpers/test-server.js b/test/helpers/test-server.js index f9284c4fca..85de3777f7 100644 --- a/test/helpers/test-server.js +++ b/test/helpers/test-server.js @@ -40,9 +40,19 @@ function startFullSetup(config, options, done) { server = new Server(compiler, options); - const port = Object.prototype.hasOwnProperty.call(options, 'port') - ? options.port - : 8080; + // originally the fallback default was 8080, but it should be + // undefined so that the server.listen method can choose it for us, + // and thus prevent port mapping collision between tests + let port; + if (Object.prototype.hasOwnProperty.call(options, 'port')) { + port = options.port; + } else if (Object.prototype.hasOwnProperty.call(options, 'socket')) { + port = options.socket; + } else { + // TODO: remove this when findPort is implemented in the server.listen method + port = 8080; + } + const host = Object.prototype.hasOwnProperty.call(options, 'host') ? options.host : 'localhost'; @@ -65,10 +75,12 @@ function startFullSetup(config, options, done) { function startAwaitingCompilationFullSetup(config, options, done) { let readyCount = 0; - const ready = () => { + let err; + const ready = (e) => { + err = e instanceof Error || (typeof e === 'object' && e.code) ? e : err; readyCount += 1; if (readyCount === 2) { - done(); + done(err); } }; diff --git a/test/server/onListening-option.test.js b/test/server/onListening-option.test.js index d6d66b1565..2d8b86dc3e 100644 --- a/test/server/onListening-option.test.js +++ b/test/server/onListening-option.test.js @@ -1,32 +1,71 @@ 'use strict'; +const { unlink } = require('fs'); +const { join } = require('path'); const testServer = require('../helpers/test-server'); const config = require('../fixtures/simple-config/webpack.config'); const port = require('../ports-map')['onListening-option']; +const { skipTestOnWindows } = require('../helpers/conditional-test'); describe('onListening option', () => { - let onListeningIsRunning = false; - - beforeAll((done) => { - testServer.start( - config, - { - onListening: (devServer) => { - if (!devServer) { - throw new Error('webpack-dev-server is not defined'); - } - - onListeningIsRunning = true; + describe('with host and port', () => { + let onListeningIsRunning = false; + + beforeAll((done) => { + testServer.start( + config, + { + onListening: (devServer) => { + if (!devServer) { + throw new Error('webpack-dev-server is not defined'); + } + + onListeningIsRunning = true; + }, + port, }, - port, - }, - done - ); + done + ); + }); + + afterAll(testServer.close); + + it('should run onListening callback', () => { + expect(onListeningIsRunning).toBe(true); + }); }); - afterAll(testServer.close); + describe('with Unix socket', () => { + if (skipTestOnWindows('Unix sockets are not supported on Windows')) { + return; + } + + const socketPath = join('.', 'onListening.webpack.sock'); + let onListeningIsRunning = false; + + beforeAll((done) => { + testServer.start( + config, + { + onListening: (devServer) => { + if (!devServer) { + throw new Error('webpack-dev-server is not defined'); + } + + onListeningIsRunning = true; + }, + socket: socketPath, + }, + done + ); + }); + + afterAll(testServer.close); + + it('should run onListening callback', (done) => { + expect(onListeningIsRunning).toBe(true); - it('should runs onListening callback', () => { - expect(onListeningIsRunning).toBe(true); + unlink(socketPath, done); + }); }); }); diff --git a/test/server/socket-option.test.js b/test/server/socket-option.test.js new file mode 100644 index 0000000000..4699117d27 --- /dev/null +++ b/test/server/socket-option.test.js @@ -0,0 +1,164 @@ +'use strict'; + +const http = require('http'); +const net = require('net'); +const fs = require('fs'); +const path = require('path'); +const webpack = require('webpack'); +const testServer = require('../helpers/test-server'); +const config = require('../fixtures/simple-config/webpack.config'); +const Server = require('../../lib/Server'); +const { skipTestOnWindows } = require('../helpers/conditional-test'); + +describe('socket', () => { + const socketPath = path.join('.', 'socket-option.webpack.sock'); + let server = null; + + describe('path to a non-existent file', () => { + let err; + beforeAll((done) => { + server = testServer.start( + config, + { + socket: socketPath, + }, + (e) => { + err = e; + done(); + } + ); + }); + + it('should work as Unix socket or error on windows', (done) => { + if (process.platform === 'win32') { + expect(err.code).toEqual('EACCES'); + done(); + } else { + const clientSocket = new net.Socket(); + clientSocket.connect({ path: socketPath }, () => { + // this means the connection was made successfully + expect(true).toBeTruthy(); + done(); + }); + } + }); + + afterAll((done) => { + testServer.close(() => { + fs.unlink(socketPath, done); + }); + }); + }); + + describe('path to existent, unused file', () => { + if (skipTestOnWindows('Unix sockets are not supported on Windows')) { + return; + } + + beforeAll((done) => { + fs.writeFileSync(socketPath, ''); + server = testServer.start( + config, + { + socket: socketPath, + }, + done + ); + }); + + it('should work as Unix socket', (done) => { + const clientSocket = new net.Socket(); + clientSocket.connect({ path: socketPath }, () => { + // this means the connection was made successfully + expect(true).toBeTruthy(); + done(); + }); + }); + + afterAll((done) => { + testServer.close(() => { + fs.unlink(socketPath, done); + }); + }); + }); + + describe('path to existent file with listening server', () => { + if (skipTestOnWindows('Unix sockets are not supported on Windows')) { + return; + } + + let dummyServer; + const sockets = new Set(); + beforeAll((done) => { + dummyServer = http.createServer(); + dummyServer.listen(socketPath, 511, () => { + done(); + }); + dummyServer.on('connection', (socket) => { + sockets.add(socket); + socket.on('close', () => { + sockets.delete(socket); + }); + }); + }); + + it('should work as Unix socket', (done) => { + server = testServer.start( + config, + { + socket: socketPath, + }, + (err) => { + expect(err).not.toBeNull(); + expect(err).not.toBeUndefined(); + expect(err.message).toEqual('This socket is already used'); + server.close(done); + } + ); + }); + + afterAll((done) => { + // get rid of connected sockets on the dummyServer + for (const socket of sockets.values()) { + socket.destroy(); + } + dummyServer.close(() => { + fs.unlink(socketPath, done); + }); + }); + }); + + describe('path to existent, unused file', () => { + if (skipTestOnWindows('Unix sockets are not supported on Windows')) { + return; + } + + let devServer; + const options = { + socket: socketPath, + }; + beforeAll(() => { + fs.writeFileSync(socketPath, ''); + }); + + // this test is significant because previously the callback was called + // twice if a file at the given socket path already existed, but + // could be removed + it('should only call server.listen callback once', (done) => { + const compiler = webpack(config); + + devServer = new Server(compiler, options); + const onListen = jest.fn(); + // eslint-disable-next-line no-undefined + devServer.listen(options.socket, undefined, onListen); + setTimeout(() => { + expect(onListen).toBeCalledTimes(1); + done(); + }, 10000); + }); + + afterAll((done) => { + devServer.close(done); + }); + }); +}); From 4e52766d1d1a52b8da847ef014971f2469c7698e Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Tue, 25 Jun 2019 11:36:06 -0500 Subject: [PATCH 2/7] refactor(server): added startUnixSocket helper --- lib/Server.js | 52 ++--------------------------- lib/utils/startUnixSocket.js | 65 ++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 50 deletions(-) create mode 100644 lib/utils/startUnixSocket.js diff --git a/lib/Server.js b/lib/Server.js index b538de9c35..a20218ce0d 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -6,7 +6,6 @@ func-names */ const fs = require('fs'); -const net = require('net'); const path = require('path'); const tls = require('tls'); const url = require('url'); @@ -36,6 +35,7 @@ const routes = require('./utils/routes'); const getSocketServerImplementation = require('./utils/getSocketServerImplementation'); const handleStdin = require('./utils/handleStdin'); const tryParseInt = require('./utils/tryParseInt'); +const startUnixSocket = require('./utils/startUnixSocket'); const schema = require('./options.json'); // Workaround for node ^8.6.0, ^9.0.0 @@ -776,55 +776,7 @@ class Server { // set this so that status helper can identify how the project is being run correctly this.options.socket = socket; - const chmodSocket = (done) => { - // chmod 666 (rw rw rw) - octal - const READ_WRITE = 438; - fs.chmod(socket, READ_WRITE, done); - }; - - const startSocket = () => { - this.listeningApp.on('error', (err) => { - userCallback(err); - }); - - this.listeningApp.listen(socket, hostname, (err) => { - if (err) { - fullCallback(err); - } else { - chmodSocket(fullCallback); - } - }); - }; - - fs.access(socket, fs.constants.F_OK, (err) => { - if (err) { - // file does not exist - startSocket(); - } else { - // file exists - - const clientSocket = new net.Socket(); - - clientSocket.on('error', (err) => { - if (err.code === 'ECONNREFUSED' || err.code === 'ENOTSOCK') { - // No other server listening on this socket so it can be safely removed - fs.unlinkSync(socket); - - startSocket(); - } - }); - - clientSocket.connect({ path: socket }, () => { - // if a client socket successfully connects to the given socket path, - // it means that the socket is in use - const err = new Error('This socket is already used'); - clientSocket.destroy(); - // do not call onListening or the setup method, since the server - // cannot start listening on a used socket - userCallback(err); - }); - } - }); + startUnixSocket(this.listeningApp, socket, fullCallback, userCallback); } else { this.listeningApp.listen(port, hostname, fullCallback); } diff --git a/lib/utils/startUnixSocket.js b/lib/utils/startUnixSocket.js new file mode 100644 index 0000000000..99a5cf9e5e --- /dev/null +++ b/lib/utils/startUnixSocket.js @@ -0,0 +1,65 @@ +'use strict'; + +const fs = require('fs'); +const net = require('net'); + +function startUnixSocket( + listeningApp, + socket, + onListeningCallback, + errorCallback +) { + const chmodSocket = (done) => { + // chmod 666 (rw rw rw) - octal + const READ_WRITE = 438; + fs.chmod(socket, READ_WRITE, done); + }; + + const startSocket = () => { + listeningApp.on('error', (err) => { + errorCallback(err); + }); + + // 511 is the default value for the server.listen backlog parameter + // https://nodejs.org/api/net.html#net_server_listen + listeningApp.listen(socket, 511, (err) => { + if (err) { + onListeningCallback(err); + } else { + chmodSocket(onListeningCallback); + } + }); + }; + + fs.access(socket, fs.constants.F_OK, (err) => { + if (err) { + // file does not exist + startSocket(); + } else { + // file exists + + const clientSocket = new net.Socket(); + + clientSocket.on('error', (err) => { + if (err.code === 'ECONNREFUSED' || err.code === 'ENOTSOCK') { + // No other server listening on this socket so it can be safely removed + fs.unlinkSync(socket); + + startSocket(); + } + }); + + clientSocket.connect({ path: socket }, () => { + // if a client socket successfully connects to the given socket path, + // it means that the socket is in use + const err = new Error('This socket is already used'); + clientSocket.destroy(); + // do not call onListening or the setup method, since the server + // cannot start listening on a used socket + errorCallback(err); + }); + } + }); +} + +module.exports = startUnixSocket; From 30c2d4f3e9bb30c32ef7c9ccc4ee8d685a65d606 Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Tue, 25 Jun 2019 12:43:47 -0500 Subject: [PATCH 3/7] test(server): added unix socket helper tests --- lib/Server.js | 17 ++- lib/utils/startUnixSocket.js | 24 ++-- test/helpers/test-unix-socket.js | 26 ++++ test/server/socket-option.test.js | 27 +---- test/server/utils/startUnixSocket.test.js | 139 ++++++++++++++++++++++ 5 files changed, 200 insertions(+), 33 deletions(-) create mode 100644 test/helpers/test-unix-socket.js create mode 100644 test/server/utils/startUnixSocket.test.js diff --git a/lib/Server.js b/lib/Server.js index a20218ce0d..12ed97e758 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -755,16 +755,20 @@ class Server { } }; - const onListeningCallback = () => { + const listenCallback = () => { if (typeof this.options.onListening === 'function') { this.options.onListening(this); } }; - const fullCallback = (err) => { + const setupAndListenCallback = () => { setupCallback(); + listenCallback(); + }; + + const fullCallback = (err) => { + setupAndListenCallback(); userCallback(err); - onListeningCallback(); }; // try to follow the Node standard in terms of deciding @@ -776,7 +780,12 @@ class Server { // set this so that status helper can identify how the project is being run correctly this.options.socket = socket; - startUnixSocket(this.listeningApp, socket, fullCallback, userCallback); + startUnixSocket( + this.listeningApp, + socket, + setupAndListenCallback, + userCallback + ); } else { this.listeningApp.listen(port, hostname, fullCallback); } diff --git a/lib/utils/startUnixSocket.js b/lib/utils/startUnixSocket.js index 99a5cf9e5e..16e09a81d3 100644 --- a/lib/utils/startUnixSocket.js +++ b/lib/utils/startUnixSocket.js @@ -3,12 +3,20 @@ const fs = require('fs'); const net = require('net'); +// userCallback should always be called to indicate that either the server has +// started listening, or an error was thrown. +// setupAndListenCallback should only be called if the server starts listening function startUnixSocket( listeningApp, socket, - onListeningCallback, - errorCallback + setupAndListenCallback, + userCallback ) { + const fullCallback = (err) => { + setupAndListenCallback(); + userCallback(err); + }; + const chmodSocket = (done) => { // chmod 666 (rw rw rw) - octal const READ_WRITE = 438; @@ -17,22 +25,22 @@ function startUnixSocket( const startSocket = () => { listeningApp.on('error', (err) => { - errorCallback(err); + userCallback(err); }); // 511 is the default value for the server.listen backlog parameter // https://nodejs.org/api/net.html#net_server_listen listeningApp.listen(socket, 511, (err) => { if (err) { - onListeningCallback(err); + fullCallback(err); } else { - chmodSocket(onListeningCallback); + chmodSocket(fullCallback); } }); }; - fs.access(socket, fs.constants.F_OK, (err) => { - if (err) { + fs.access(socket, fs.constants.F_OK, (e) => { + if (e) { // file does not exist startSocket(); } else { @@ -56,7 +64,7 @@ function startUnixSocket( clientSocket.destroy(); // do not call onListening or the setup method, since the server // cannot start listening on a used socket - errorCallback(err); + userCallback(err); }); } }); diff --git a/test/helpers/test-unix-socket.js b/test/helpers/test-unix-socket.js new file mode 100644 index 0000000000..050582077b --- /dev/null +++ b/test/helpers/test-unix-socket.js @@ -0,0 +1,26 @@ +'use strict'; + +const http = require('http'); + +const TestUnixSocket = class TestUnixSocket { + constructor() { + this.server = http.createServer(); + this.sockets = new Set(); + this.server.on('connection', (socket) => { + this.sockets.add(socket); + socket.on('close', () => { + this.sockets.delete(socket); + }); + }); + } + + close(done) { + // get rid of connected sockets + for (const socket of this.sockets.values()) { + socket.destroy(); + } + this.server.close(done); + } +}; + +module.exports = TestUnixSocket; diff --git a/test/server/socket-option.test.js b/test/server/socket-option.test.js index 4699117d27..0bdd989554 100644 --- a/test/server/socket-option.test.js +++ b/test/server/socket-option.test.js @@ -1,14 +1,14 @@ 'use strict'; -const http = require('http'); const net = require('net'); const fs = require('fs'); const path = require('path'); const webpack = require('webpack'); const testServer = require('../helpers/test-server'); +const TestUnixSocket = require('../helpers/test-unix-socket'); +const { skipTestOnWindows } = require('../helpers/conditional-test'); const config = require('../fixtures/simple-config/webpack.config'); const Server = require('../../lib/Server'); -const { skipTestOnWindows } = require('../helpers/conditional-test'); describe('socket', () => { const socketPath = path.join('.', 'socket-option.webpack.sock'); @@ -87,19 +87,12 @@ describe('socket', () => { return; } - let dummyServer; - const sockets = new Set(); + let testUnixSocket; beforeAll((done) => { - dummyServer = http.createServer(); - dummyServer.listen(socketPath, 511, () => { + testUnixSocket = new TestUnixSocket(); + testUnixSocket.server.listen(socketPath, 511, () => { done(); }); - dummyServer.on('connection', (socket) => { - sockets.add(socket); - socket.on('close', () => { - sockets.delete(socket); - }); - }); }); it('should work as Unix socket', (done) => { @@ -118,21 +111,13 @@ describe('socket', () => { }); afterAll((done) => { - // get rid of connected sockets on the dummyServer - for (const socket of sockets.values()) { - socket.destroy(); - } - dummyServer.close(() => { + testUnixSocket.close(() => { fs.unlink(socketPath, done); }); }); }); describe('path to existent, unused file', () => { - if (skipTestOnWindows('Unix sockets are not supported on Windows')) { - return; - } - let devServer; const options = { socket: socketPath, diff --git a/test/server/utils/startUnixSocket.test.js b/test/server/utils/startUnixSocket.test.js new file mode 100644 index 0000000000..dd457a031d --- /dev/null +++ b/test/server/utils/startUnixSocket.test.js @@ -0,0 +1,139 @@ +'use strict'; + +const net = require('net'); +const fs = require('fs'); +const path = require('path'); +const TestUnixSocket = require('../../helpers/test-unix-socket'); +const { skipTestOnWindows } = require('../../helpers/conditional-test'); +const startUnixSocket = require('../../../lib/utils/startUnixSocket'); + +describe('startUnixSocket', () => { + const socketPath = path.join('.', 'startUnixSocket.webpack.sock'); + let testUnixSocket = null; + + describe('path to a non-existent file', () => { + let err; + beforeAll((done) => { + testUnixSocket = new TestUnixSocket(); + startUnixSocket( + testUnixSocket.server, + socketPath, + () => {}, + (e) => { + err = e; + done(); + } + ); + }); + + it('should work as Unix socket or error on windows', (done) => { + if (process.platform === 'win32') { + expect(err.code).toEqual('EACCES'); + done(); + } else { + const clientSocket = new net.Socket(); + clientSocket.connect({ path: socketPath }, () => { + // this means the connection was made successfully + expect(true).toBeTruthy(); + done(); + }); + } + }); + + afterAll((done) => { + testUnixSocket.close(() => { + fs.unlink(socketPath, done); + }); + }); + }); + + describe('path to existent, unused file', () => { + if (skipTestOnWindows('Unix sockets are not supported on Windows')) { + return; + } + + beforeAll((done) => { + fs.writeFileSync(socketPath, ''); + testUnixSocket = new TestUnixSocket(); + startUnixSocket(testUnixSocket.server, socketPath, () => {}, done); + }); + + it('should work as Unix socket', (done) => { + const clientSocket = new net.Socket(); + clientSocket.connect({ path: socketPath }, () => { + // this means the connection was made successfully + expect(true).toBeTruthy(); + done(); + }); + }); + + afterAll((done) => { + testUnixSocket.close(() => { + fs.unlink(socketPath, done); + }); + }); + }); + + describe('path to existent file with listening server', () => { + if (skipTestOnWindows('Unix sockets are not supported on Windows')) { + return; + } + + let dummyUnixSocket; + beforeAll((done) => { + dummyUnixSocket = new TestUnixSocket(); + dummyUnixSocket.server.listen(socketPath, 511, () => { + done(); + }); + }); + + it('should work as Unix socket', (done) => { + testUnixSocket = new TestUnixSocket(); + startUnixSocket( + testUnixSocket.server, + socketPath, + () => {}, + (err) => { + expect(err).not.toBeNull(); + expect(err).not.toBeUndefined(); + expect(err.message).toEqual('This socket is already used'); + testUnixSocket.close(done); + } + ); + }); + + afterAll((done) => { + dummyUnixSocket.close(() => { + fs.unlink(socketPath, done); + }); + }); + }); + + describe('path to existent, unused file', () => { + beforeAll(() => { + fs.writeFileSync(socketPath, ''); + testUnixSocket = new TestUnixSocket(); + }); + + // this test is significant because previously the callback was called + // twice if a file at the given socket path already existed, but + // could be removed + it('should only call server.listen callback once', (done) => { + const userCallback = jest.fn(); + startUnixSocket( + testUnixSocket.server, + socketPath, + () => {}, + userCallback + ); + setTimeout(() => { + expect(userCallback).toBeCalledTimes(1); + done(); + }, 10000); + }); + + afterAll((done) => { + testUnixSocket.close(done); + }); + }); +}); From bbdd01e311ed438de97e5303040407a41c92339e Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Tue, 25 Jun 2019 13:02:18 -0500 Subject: [PATCH 4/7] test(server): fixed unix socket helper close method --- test/helpers/test-unix-socket.js | 12 ++++++++---- test/server/socket-option.test.js | 2 +- test/server/utils/startUnixSocket.test.js | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/test/helpers/test-unix-socket.js b/test/helpers/test-unix-socket.js index 050582077b..d2c504b62f 100644 --- a/test/helpers/test-unix-socket.js +++ b/test/helpers/test-unix-socket.js @@ -15,11 +15,15 @@ const TestUnixSocket = class TestUnixSocket { } close(done) { - // get rid of connected sockets - for (const socket of this.sockets.values()) { - socket.destroy(); + if (this.server.listening) { + // get rid of connected sockets + for (const socket of this.sockets.values()) { + socket.destroy(); + } + this.server.close(done); + } else { + done(); } - this.server.close(done); } }; diff --git a/test/server/socket-option.test.js b/test/server/socket-option.test.js index 0bdd989554..a933097bb1 100644 --- a/test/server/socket-option.test.js +++ b/test/server/socket-option.test.js @@ -95,7 +95,7 @@ describe('socket', () => { }); }); - it('should work as Unix socket', (done) => { + it('should throw already used error', (done) => { server = testServer.start( config, { diff --git a/test/server/utils/startUnixSocket.test.js b/test/server/utils/startUnixSocket.test.js index dd457a031d..64caeb4866 100644 --- a/test/server/utils/startUnixSocket.test.js +++ b/test/server/utils/startUnixSocket.test.js @@ -87,7 +87,7 @@ describe('startUnixSocket', () => { }); }); - it('should work as Unix socket', (done) => { + it('should throw already used error', (done) => { testUnixSocket = new TestUnixSocket(); startUnixSocket( testUnixSocket.server, From 8da9a8c991de7f0f78bb0d7747039d99c6a5476c Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Tue, 25 Jun 2019 14:31:45 -0500 Subject: [PATCH 5/7] feat(server): made startUnixSocket take one callback --- lib/Server.js | 17 +++------- lib/utils/startUnixSocket.js | 25 +++------------ test/server/utils/startUnixSocket.test.js | 39 +++++++---------------- 3 files changed, 21 insertions(+), 60 deletions(-) diff --git a/lib/Server.js b/lib/Server.js index 12ed97e758..4846bf5ee2 100644 --- a/lib/Server.js +++ b/lib/Server.js @@ -755,20 +755,16 @@ class Server { } }; - const listenCallback = () => { + const onListeningCallback = () => { if (typeof this.options.onListening === 'function') { this.options.onListening(this); } }; - const setupAndListenCallback = () => { - setupCallback(); - listenCallback(); - }; - const fullCallback = (err) => { - setupAndListenCallback(); + setupCallback(); userCallback(err); + onListeningCallback(); }; // try to follow the Node standard in terms of deciding @@ -780,12 +776,7 @@ class Server { // set this so that status helper can identify how the project is being run correctly this.options.socket = socket; - startUnixSocket( - this.listeningApp, - socket, - setupAndListenCallback, - userCallback - ); + startUnixSocket(this.listeningApp, socket, fullCallback); } else { this.listeningApp.listen(port, hostname, fullCallback); } diff --git a/lib/utils/startUnixSocket.js b/lib/utils/startUnixSocket.js index 16e09a81d3..012167803d 100644 --- a/lib/utils/startUnixSocket.js +++ b/lib/utils/startUnixSocket.js @@ -3,20 +3,7 @@ const fs = require('fs'); const net = require('net'); -// userCallback should always be called to indicate that either the server has -// started listening, or an error was thrown. -// setupAndListenCallback should only be called if the server starts listening -function startUnixSocket( - listeningApp, - socket, - setupAndListenCallback, - userCallback -) { - const fullCallback = (err) => { - setupAndListenCallback(); - userCallback(err); - }; - +function startUnixSocket(listeningApp, socket, cb) { const chmodSocket = (done) => { // chmod 666 (rw rw rw) - octal const READ_WRITE = 438; @@ -25,16 +12,16 @@ function startUnixSocket( const startSocket = () => { listeningApp.on('error', (err) => { - userCallback(err); + cb(err); }); // 511 is the default value for the server.listen backlog parameter // https://nodejs.org/api/net.html#net_server_listen listeningApp.listen(socket, 511, (err) => { if (err) { - fullCallback(err); + cb(err); } else { - chmodSocket(fullCallback); + chmodSocket(cb); } }); }; @@ -62,9 +49,7 @@ function startUnixSocket( // it means that the socket is in use const err = new Error('This socket is already used'); clientSocket.destroy(); - // do not call onListening or the setup method, since the server - // cannot start listening on a used socket - userCallback(err); + cb(err); }); } }); diff --git a/test/server/utils/startUnixSocket.test.js b/test/server/utils/startUnixSocket.test.js index 64caeb4866..0fffd086e9 100644 --- a/test/server/utils/startUnixSocket.test.js +++ b/test/server/utils/startUnixSocket.test.js @@ -15,15 +15,10 @@ describe('startUnixSocket', () => { let err; beforeAll((done) => { testUnixSocket = new TestUnixSocket(); - startUnixSocket( - testUnixSocket.server, - socketPath, - () => {}, - (e) => { - err = e; - done(); - } - ); + startUnixSocket(testUnixSocket.server, socketPath, (e) => { + err = e; + done(); + }); }); it('should work as Unix socket or error on windows', (done) => { @@ -55,7 +50,7 @@ describe('startUnixSocket', () => { beforeAll((done) => { fs.writeFileSync(socketPath, ''); testUnixSocket = new TestUnixSocket(); - startUnixSocket(testUnixSocket.server, socketPath, () => {}, done); + startUnixSocket(testUnixSocket.server, socketPath, done); }); it('should work as Unix socket', (done) => { @@ -89,17 +84,12 @@ describe('startUnixSocket', () => { it('should throw already used error', (done) => { testUnixSocket = new TestUnixSocket(); - startUnixSocket( - testUnixSocket.server, - socketPath, - () => {}, - (err) => { - expect(err).not.toBeNull(); - expect(err).not.toBeUndefined(); - expect(err.message).toEqual('This socket is already used'); - testUnixSocket.close(done); - } - ); + startUnixSocket(testUnixSocket.server, socketPath, (err) => { + expect(err).not.toBeNull(); + expect(err).not.toBeUndefined(); + expect(err.message).toEqual('This socket is already used'); + testUnixSocket.close(done); + }); }); afterAll((done) => { @@ -120,12 +110,7 @@ describe('startUnixSocket', () => { // could be removed it('should only call server.listen callback once', (done) => { const userCallback = jest.fn(); - startUnixSocket( - testUnixSocket.server, - socketPath, - () => {}, - userCallback - ); + startUnixSocket(testUnixSocket.server, socketPath, userCallback); setTimeout(() => { expect(userCallback).toBeCalledTimes(1); done(); From b3524489436867461f0edeb704ea7e094a033608 Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Thu, 8 Aug 2019 16:58:33 -0500 Subject: [PATCH 6/7] refactor(server): use promisify to make unix socket async await --- lib/utils/startUnixSocket.js | 48 +++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/lib/utils/startUnixSocket.js b/lib/utils/startUnixSocket.js index 012167803d..9bf850c2ec 100644 --- a/lib/utils/startUnixSocket.js +++ b/lib/utils/startUnixSocket.js @@ -2,8 +2,11 @@ const fs = require('fs'); const net = require('net'); +const { promisify } = require('util'); -function startUnixSocket(listeningApp, socket, cb) { +const accessAsync = promisify(fs.access); + +async function startUnixSocket(listeningApp, socket, cb) { const chmodSocket = (done) => { // chmod 666 (rw rw rw) - octal const READ_WRITE = 438; @@ -26,33 +29,34 @@ function startUnixSocket(listeningApp, socket, cb) { }); }; - fs.access(socket, fs.constants.F_OK, (e) => { - if (e) { - // file does not exist - startSocket(); - } else { - // file exists + try { + await accessAsync(socket, fs.constants.F_OK); + } catch (e) { + // file does not exist + startSocket(); + return; + } - const clientSocket = new net.Socket(); + // file exists - clientSocket.on('error', (err) => { - if (err.code === 'ECONNREFUSED' || err.code === 'ENOTSOCK') { - // No other server listening on this socket so it can be safely removed - fs.unlinkSync(socket); + const clientSocket = new net.Socket(); - startSocket(); - } - }); + clientSocket.on('error', (err) => { + if (err.code === 'ECONNREFUSED' || err.code === 'ENOTSOCK') { + // No other server listening on this socket so it can be safely removed + fs.unlinkSync(socket); - clientSocket.connect({ path: socket }, () => { - // if a client socket successfully connects to the given socket path, - // it means that the socket is in use - const err = new Error('This socket is already used'); - clientSocket.destroy(); - cb(err); - }); + startSocket(); } }); + + clientSocket.connect({ path: socket }, () => { + // if a client socket successfully connects to the given socket path, + // it means that the socket is in use + const err = new Error('This socket is already used'); + clientSocket.destroy(); + cb(err); + }); } module.exports = startUnixSocket; From 8577191642702734117a4e1a7e32cdba789fd6e9 Mon Sep 17 00:00:00 2001 From: Kirill Nagaitsev Date: Thu, 8 Aug 2019 17:57:19 -0500 Subject: [PATCH 7/7] test(server): reduce time to wait for unix socket listening --- test/server/utils/startUnixSocket.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/server/utils/startUnixSocket.test.js b/test/server/utils/startUnixSocket.test.js index 0fffd086e9..e0ab7b899a 100644 --- a/test/server/utils/startUnixSocket.test.js +++ b/test/server/utils/startUnixSocket.test.js @@ -114,7 +114,7 @@ describe('startUnixSocket', () => { setTimeout(() => { expect(userCallback).toBeCalledTimes(1); done(); - }, 10000); + }, 3000); }); afterAll((done) => {