Hi! Thanks for the pull request 👍

I'm not sure about merging this. I think I'd rather have the users explicitly call close(), instead of implicitly closing the connection when there is nothing left in the event loop.

I see there are a lot of ❤️ on the pull request, what is the opinion of others?

Hi, @darrachequesne -- without this patch, it can be extremely difficult for users of libraries which use SocketIO to ensure that their Node programs will exit "naturally" - i.e. without explicitly calling process.exit(). This means, in turn then, that developers of libraries using SocketIO have to add a specific close function, and that they have to expose their API consumers to the underlying implementation detail that there is a SocketIO socket in use which needs to be closed.

This is particularly insidious, as the implementation details of an underlying library should not be of concern to API consumers -- but the lack of an unref() for use by the library developer forces the API consumer to handle socket closing. In turn this means that libraries that are implemented with Node sockets cannot be re-implemented with SocketIO without changing their externally-facing API library.

I believe that it should be a design goal of SocketIO to allow library authors to implement the same libraries they can implement with native NodeJS sockets, without requiring a library API change. NodeJS explicitly added Timer and Socket .unref() because not doing so made program-exit/cleanup difficult or unreliable in some circumstances. We should allow SocketIO users the same flexibility, not dictate the type of flow control they implement in their programs.

FWIW -- node-redis and net-websocket-polyfill expose an unref() method and it is on the roadmap for simple-websockets.

That makes sense, thanks a lot for the clear explanation 👍

I encountered an exception when restricting the transports:

const socket = eio("http://localhost:3000", {
  autoUnref: true,
  transports: ["websocket"]
});

throws:

    this.ws._socket.unref();
                    ^

TypeError: Cannot read property 'unref' of null
    at WS.doUnref (/home/damien/git/socket.io/engine.io-client/lib/transports/websocket.js:104:21)
    at WS.unref (/home/damien/git/socket.io/engine.io-client/lib/transport.js:55:10)
    at Socket.setTransport (/home/damien/git/socket.io/engine.io-client/lib/socket.js:221:17)
    at Socket.open (/home/damien/git/socket.io/engine.io-client/lib/socket.js:185:10)

(I think we need to wait for the onopen trigger here)

With HTTP long-polling only, it does not seem to work either (the process does not stop):

const socket = eio("http://localhost:3000", {
  autoUnref: true,
  transports: ["polling"]
});

I think we'll have to dig into the source of the xmlhttprequest-ssl package here.

Would you have time to check?

Thanks @wesgarland that's a great way of putting it!

@darrachequesne I haven't played with restricted transports yet. I'll take a look next chance and get back to you with my ideas. 👍

I was able to reproduce the issue when the transport layer is specified as websocket only and the above commit fixes it. I spent a bit of time poking into the polling case but wasn't able to reproduce it.

@darrachequesne I usually use wtfnode to diagnose what exactly is hanging up the event loop. Perhaps you could post your test case or the output of wtfnode to show what exactly is the source of the problem?

@KCErb here's my code:

const eio = require(".");
const socket = eio("http://localhost:3000", {
  autoUnref: true,
  transports: ["websocket"]
});

socket.on("open", () => {
  console.log("open");
});

setTimeout(() => {
  console.log("process should exit now");
}, 2000);

With transports: ["websocket"], the process stops regardless of the status of the connection (:ok_hand:):

open
process should exit now

Process finished with exit code 0

But with transports: ["polling"] and when the connection is successfully established, the process stays alive. Is that expected? Here's the output of wtfnode:

open
process should exit now
[WTF Node?] open handles:
- File descriptors: (note: stdio always exists)
  - fd 1 (stdio)
  - fd 2 (stdio)
- Sockets:
  - 127.0.0.1:40052 -> 127.0.0.1:3000

Interesting, I ran your code with transports: ["polling"] on my branch and it exits just fine. I tried it on nodes at 10.x, 12.x, and 14.x. I also tried re-installing all dependencies at those versions but never got it to hang.

You mentioned xmlhttprequest-ssl mine is at 1.5.5? Not sure what else to check.

Merged as 6551683.

Some tests were added here: https://github.com/socketio/engine.io-client/blob/master/test/node.js

To make the test with HTTP long-polling only pass, it was necessary to edit the xmlhttprequest-ssl package here:

@KCErb could you please check if everything is OK for you?

Yup, works for me. Thanks for accepting and adding those tests too. I wasn't sure how best to test it all, nice to see your solution!

Now I'm really excited to see this get paired with socketio/socket.io-client#1442 :)

@wesgarland "...but the lack of an unref() for use by the library developer forces the API consumer to handle socket closing." to me this argument is not convincing. I will argue that if you (the consumer) use socket.io, you should be aware that you need to close it when you are done just like when you open a database connection you should close the connection when you are finished.

@darrachequesne @wesgarland Although I was not convinced that argument I did find node-redis explained unref purpose. His explanation made sense to me, "This is commonly the case for some sort of script where you are doing a batch query, or a test that connects to Redis.", i.e. for a short-lived script it is convenient.

But I also find that unref was also proposed to ioredis and author seemed to like the idea and agreed to add it but he finally rejected it.