'close' listener stop working with socket.removeAllListeners('end')

[acappella2017]

• Version:v10.13.0
• Platform:Darwin Kernel Version 18.2.0
• Subsystem:TLSSocket

Run below snippet on Node 10, the close listener will never be trigged.
If we comment out the line socket.removeAllListeners('end');, The close listener is trigged as expected.

But on Nodejs 8, close listener works with or without the removeAllListeners call.

const tls = require('tls');

const options = {
  host : "www.baidu.com",
  port: 443,
};

const socket = tls.connect(options, () => {
  console.log('===client connected');
  socket.end();
});

socket.removeAllListeners('end');

socket.on('end', () => {
  console.log('===client ends');
});

socket.on('close', ()=> {
  console.log('===client close' );
});

Output in Nodejs 10:

===client connected
===client ends

Output in Nodejs 8:

===client connected
===client ends
===client close

I believe this issue is the root cause of ldapjs/node-ldapjs#483

[addaleax]

This is most likely happening because we removed the event that we used internally in #18607.

This issue should be fixed once you use removeListener() instead of removeAllListeners()?

[acappella2017]

So the close event is trigged by the internal end listener, and all the end listeners are removed by the removeAllListeners() call?

Can we do something to prevent the internal end listener to be removed? People may not understand all the inner things .

[sam-github]

removeAllListeners() is supposed to remove all listeners. Its not called "remove all my listeners"! Node.js uses events a lot internally, removing all the listeners from a node EventEmitter, or pretty much any EventEmitter - including user-land ones, is likely to cause chaos (aka "undefined behaviour"). The form of the chaos will vary between releases.

I looked at https://github.com/joyent/node-ldapjs, whoever wrote it seemed to have some trouble getting consistent behaviour from socket events, and is using removeAllListeners() repeatedly to try and clear state. Its entirely possible this was an attempt to work around bugs in now-ancient node versions (some comments alude to inconsistencies between TLSSocket and Socket), but its equally likely it stems from misunderstandings or frustrations about the life cycle of sockets. Anyhow, its using a sledgehammer to find-tune a radio, it was going to break eventually.

[addaleax]

So … I’m not sure, but is there a specific reason you’re using removeAllListeners()? You shouldn’t usually be doing that if you don’t own the object you’re calling it on, because of this situation – you can always remove your own listeners, right?

[acappella2017]

I'm just innocent user of node-ldapjs who want to make our project work on Node 10..

@sam-github, Thanks for your detailed explanation. I'm not saying we should change the behavior of removeAllListeners...

I'm just wandering if there is better way to help developers to avoid things like this.

In Node 8, the close event for TLS socket doesn't depends on the end event. It depends on the internal _socketEnd event instead. So removing all the end listeners does no harm. (@addaleax, please correct me if it is not true.) But after them upgrade to Node 10, removeAllListeners('end') will make the close listener stop working. It is really hard for developers to find it is the call to removeAllListeners breaks things.

Remember that People may do wrong things. They will be induced to call removeAllListeners
especially on cleanup phrase. I saw a similar issue nodejs/node-v0.x-archive#8615, (long time ago) so node-ldapjs is not the only one who uses removeAllListeners and breaks the close event.

[sam-github]

I'm just wandering if there is better way to help developers to avoid things like this.

Node has slowly been becoming harder to break, but removing listeners from events that are not yours, removing methods from objects, reassigning values to globals, etc., can cause a lot of damage.

New methods, for example, sometimes use private symbols to try and hide them from API users, more values are const, more properties are read-only.

Its possible there will be a drift towards node not using the documented object events internally, but that's a big change, and I wouldn't expect it to happen quickly.

Wrt. to this specific issue, I don't believe it to be a bug, I think that node-ldapjs is depending on undefined behaviour, and it was bound to break eventually, and should be fixed. @acappella2017, I know you were a bystander here, but if you need LDAP on recent node versions, you might be forced to get involved in that project.

Note that @lpinca knew that removing '_socketEnd' in https://github.com/nodejs/node/pull/19241/files was a breaking change. It was marked semver-major, so this is not a "regression", its an intentional change that fixed #19166. You can comment on the PR that introduced the change, perhaps there was another way to fix it, but given that its semver-major, keeping a unwanted internal event around indefinitely, to keep node-ldapjs on life support even though its misusing the Node API seems like not a good idea to me, personally.

[acappella2017]

@sam-github Thanks for your response. I will try to get a fix from node-ldapjs side.

Using the documented objects internally sounds like a mix of user mode and kernel mode to me..So I 'm 100 % with the idea of not using them. (easy to say but hard to do)

BTW, I changed the title to 'close' listener stop working with socket.removeAllListeners('end'). Hope it is a more accurate brief for people who happens to see this issue.

[sam-github]

yes, that's a descriptive title, thanks.

[apapirovski]

Sounds like this can now be closed given that there's nothing from to be done from the Node core side. Do feel free to re-open this though if I misunderstood.