Try to load uws by default, fall back to ws

[kapouer]

Fixes: #432.

There are some test failures before/after so i'm not sure what to do with them.

[darrachequesne]

Shouldn't it be !== here?

[kapouer]

Indeed !

[kapouer]

Well the message in the gulpfile is wrong now, let me fix it.
Or not. It's all right. Somehow i'm confused about that.
Oh now i understand why - travis installs node 0.10 for testing with node 6 !
https://travis-ci.org/socketio/engine.io/jobs/182596511

[darrachequesne]

According to the comment above // keep require('ws') as separate expression for packers (browserify, etc), shouldn't it rather be wsModule = require('ws'); here?

Related: #418

[darrachequesne]

Shouldn't the "uws" in dev dependencies be removed?

[darrachequesne]

Also, the test for uws seems to fail:

[kapouer]

Forwarded test failure to https://github.com/uWebSockets/uWebSockets/issues/365.

[ghost]

var eioc = require('engine.io-client');
var listen = require('./common').listen;
var expect = require('expect.js');

describe('server', function () {
  it('should allow client reconnect after restarting (ws)', function (done) {
    var opts = { transports: ['websocket'] };
    var engine = listen(opts, function (port) {
      // these two lines make the difference between pass or fail
      engine.httpServer.close();
      engine.httpServer.listen(port);

      var socket = new eioc.Socket('ws://localhost:%d'.s(port), { transports: ['websocket'] });

      engine.once('connection', function (conn) {
        setTimeout(function () {
          conn.close();
        }, 10);
      });

      socket.once('close', function (reason) {
        expect(reason).to.be('transport close');
        done();
      });
    });
  });
});

This is the only failing test and it passes if you comment out the .close and .listen. This test is very strange and the httpServer being closed and listened on is not part of uws but engine.io itself. I cannot see why it would fail.

[ghost]

I've found the issue. Engine.io calls close on the websocket server whenever anything calls close on the httpServer. Engine.io then expects to reuse this closed server, which is not the case in uws. You need to fix this in Engine.io - you are not allowed to close a server and use it later. I would just remove those two lines since they make no sense to have in the first place.

[darrachequesne]

@alexhultman I guess it was implemented like node.js native net.Server, where you can close the server and then listen again. What do you think?

[ghost]

Engineio controls the webspcket module with handleUpgrade and close. The websocket module doesnt care aboit http listen/close in this case - it only cares about the fact that you call websocket module close followed by handleUpgrade. You cannot use a websocket module after closing it. You have to remove or replace this failing test because it fails due to invalid usage of uws.

[kapouer]

@alexhultman thank you for your clear answer.

@darrachequesne the problem happened to be, IMO, in engine.io, so i tried to fixed it properly in 04bde4c and rebased against latest master.

[kapouer]

The failure

server close should trigger on both ends upon ping timeout:
     Uncaught Error: expected 'transport error' to equal 'ping timeout'

happens on master as well, not related to this PR.

[ghost]

The ping failure is an old issue not related to uws. It happens with both ws and uws at random times due to too small timeout. Increase the timer for that case.

[kapouer]

Seems to pass, but let me remove this commit and make another PR just for that.

[ghost]

#461

[kapouer]

Yep :)

[darrachequesne]

@kapouer @alexhultman thanks!

[ghost]

Why is it an optional dependency? I heard this means it won't get downloaded by default and thus not really be used in most cases. Wouldn't it be better to make it a real dependency? Installation of uws cannot fail, it always succeeds silently. If someones server suddenly would blow up they could report an issue and use wsEngine: ws until it gets fixed.

[kapouer]

Hi,
this quote from npm's documentation explains it all: "The difference (for optionalDependencies) is that build failures do not cause installation to fail.".
So in any case npm install engine.io will try to install uws. Should it fail for any reason, it would fallback to the slow, horrible, scary native ws engine.

[ghost]

Oh, my bad then

[kapouer]

Pushing the reasoning further i would say uws could actually provide ws as a fallback in case it fails to install :)

[kapouer]

but you can consider this a new year's joke.

[ghost]

Even better, I could make it default to ws 👍

[lpinca]

@kapouer

@darrachequesne the problem happened to be, IMO, in engine.io, so i tried to fixed it properly in 04bde4c and rebased against latest master.

I disagree, when you use

const wss = new WebSocket.Server({ noServer: true });

you want an external server to be used and, in fact, ws works merely as an 'upgrade' handler when noServer is true. Since there is no server, wss.close() only closes the open sockets, but clientTracking is false so wss.close() is practically a noop and this was probably the reason why wss.close() wasn't used in the first place.

On the other hand uws always uses its internal C++ server and if I'm not wrong when you call wss.close() you also close this C++ server even if noServer is true.

Now 2.0.0 is out and I think we should at least fix #473. Something like this should work

diff --git a/lib/server.js b/lib/server.js
index 0da4c23..99dfb41 100644
--- a/lib/server.js
+++ b/lib/server.js
@@ -61,6 +61,8 @@ function Server (opts) {
       compression.threshold = 1024;
     }
   });
+
+  this.init();
 }
 
 /**
@@ -104,23 +106,24 @@ Server.prototype.clients;
  */
 
 Server.prototype.init = function () {
-  if (~this.transports.indexOf('websocket')) {
-    var wsModule;
-    try {
-      wsModule = require(this.wsEngine);
-    } catch (ex) {
-      this.wsEngine = 'ws';
-      // keep require('ws') as separate expression for packers (browserify, etc)
-      wsModule = require('ws');
-    }
-    var WebSocketServer = wsModule.Server;
-    this.ws = new WebSocketServer({
-      noServer: true,
-      clientTracking: false,
-      perMessageDeflate: this.perMessageDeflate,
-      maxPayload: this.maxHttpBufferSize
-    });
+  if (!~this.transports.indexOf('websocket')) return;
+
+  if (this.ws) this.ws.close();
+
+  var wsModule;
+  try {
+    wsModule = require(this.wsEngine);
+  } catch (ex) {
+    this.wsEngine = 'ws';
+    // keep require('ws') as separate expression for packers (browserify, etc)
+    wsModule = require('ws');
   }
+  this.ws = new wsModule.Server({
+    noServer: true,
+    clientTracking: false,
+    perMessageDeflate: this.perMessageDeflate,
+    maxPayload: this.maxHttpBufferSize
+  });
 };
 
 /**

[kapouer]

That's fine for me, as long as it works :)

[lpinca]

Another option would be to leave everything as is and specify that Server.prototype.init() should be called when passing requests.

var engine = require('engine.io');
var server = new engine.Server();

server.init();

server.on('connection', function(socket){
  socket.send('hi');
});

// …
httpServer.on('upgrade', function(req, socket, head){
  server.handleUpgrade(req, socket, head);
});
httpServer.on('request', function(req, res){
  server.handleRequest(req, res);
});