refactor(server): move socket handling into server.listen (#2061)

Author: knagaitsev

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');
@@ -116,42 +114,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) => {

lib/Server.js

@@ -34,6 +34,8 @@ 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 startUnixSocket = require('./utils/startUnixSocket');
 const schema = require('./options.json');
 
 // Workaround for node ^8.6.0, ^9.0.0
@@ -743,23 +745,54 @@ class Server {
   listen(port, hostname, fn) {
     this.hostname = hostname;
 
-    return this.listeningApp.listen(port, hostname, (err) => {
+    const setupCallback = () => {
       this.createSocketServer();
 
       if (this.options.bonjour) {
         runBonjour(this.options);
       }
 
       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;
+
+      startUnixSocket(this.listeningApp, socket, fullCallback);
+    } else {
+      this.listeningApp.listen(port, hostname, fullCallback);
+    }
+
+    return this.listeningApp;
   }
 
   close(cb) {

lib/utils/startUnixSocket.js

@@ -0,0 +1,62 @@
+'use strict';
+
+const fs = require('fs');
+const net = require('net');
+const { promisify } = require('util');
+
+const accessAsync = promisify(fs.access);
+
+async function startUnixSocket(listeningApp, socket, cb) {
+  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) => {
+      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) {
+        cb(err);
+      } else {
+        chmodSocket(cb);
+      }
+    });
+  };
+
+  try {
+    await accessAsync(socket, fs.constants.F_OK);
+  } catch (e) {
+    // file does not exist
+    startSocket();
+    return;
+  }
+
+  // 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();
+    cb(err);
+  });
+}
+
+module.exports = startUnixSocket;

test/cli/cli.test.js

@@ -10,6 +10,7 @@ const { unlinkAsync } = require('../helpers/fs');
 const testBin = require('../helpers/test-bin');
 const port1 = require('../ports-map').cli[0];
 const timer = require('../helpers/timer');
+const { skipTestOnWindows } = require('../helpers/conditional-test');
 
 const httpsCertificateDirectory = resolve(
   __dirname,
@@ -94,14 +95,14 @@ describe('CLI', () => {
       .catch(done);
   });
 
-  // 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');
 
     testBin(`--socket ${socketPath}`)
       .then((output) => {

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);
     }
   };
 

test/helpers/test-unix-socket.js

@@ -0,0 +1,30 @@
+'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) {
+    if (this.server.listening) {
+      // get rid of connected sockets
+      for (const socket of this.sockets.values()) {
+        socket.destroy();
+      }
+      this.server.close(done);
+    } else {
+      done();
+    }
+  }
+};
+
+module.exports = TestUnixSocket;

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);
+    });
   });
 });