Repository navigation
'close' listener stop working with socket.removeAllListeners('end') #24577
Description
- 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
Activity
addaleax commentedon Nov 23, 2018
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 ofremoveAllListeners()?acappella2017 commentedon Nov 23, 2018
So the
closeevent is trigged by the internalendlistener, and all theendlisteners are removed by the removeAllListeners() call?Can we do something to prevent the internal
endlistener to be removed? People may not understand all the inner things .sam-github commentedon Nov 23, 2018
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 commentedon Nov 24, 2018
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 commentedon Nov 24, 2018
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
closeevent for TLS socket doesn't depends on theendevent. It depends on the internal_socketEndevent 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 thecloselistener 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 commentedon Nov 26, 2018
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-ldapjsis 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 markedsemver-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.[-][Regression]weird behavior of socket.removeAllListeners()[/-][+]'close' listener stop working with socket.removeAllListeners('end')[/+]acappella2017 commentedon Nov 27, 2018
@sam-github Thanks for your response. I will try to get a fix from
node-ldapjsside.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 commentedon Nov 27, 2018
yes, that's a descriptive title, thanks.
apapirovski commentedon Nov 29, 2018
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.