readline: clean up event listener in onNewListener

[gibfahn]

If this is a bug, then let's just fix it.

Refs: #9447 (comment)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

readline

[gibfahn]

cc/ @STRML

@princejwesley LMK if you're okay with this landing under your name (as it's your code!)

[gibfahn]

Moved the check out of the function and handled iface being undefined, to avoid this error:

Cannot read property 'once' of undefined

===releasetest-readline-emit-keypress-events===Path: parallel/test-readline-emit-keypress-eventsreadline.js:1033iface.once('close',()=>{stream.removeListener('data',onData);});^ TypeError: Cannotreadproperty'once'ofundefinedatPassThrough.onNewListener(readline.js:1033:12)atemitTwo(events.js:125:13)atPassThrough.emit(events.js:213:7)at_addListener(events.js:248:14)atPassThrough.addListener(events.js:298:10)atPassThrough.Readable.on(_stream_readable.js:759:35)atObject.<anonymous>(/Users/gib/wrk/com/node/test/parallel/test-readline-emit-keypress-events.js:16:8)atModule._compile(module.js:569:30)atObject.Module._extensions..js(module.js:580:10)atModule.load(module.js:503:32) Command: out/Release/node/Users/gib/wrk/com/node/test/parallel/test-readline-emit-keypress-events.js

This now fails test-readline-set-raw-mode.js#L90 with:

===releasetest-readline-set-raw-mode===Path: parallel/test-readline-set-raw-modeassert.js:60thrownewerrors.AssertionError({^AssertionError[ERR_ASSERTION]: 0===1atObject.<anonymous>(/Users/gib/wrk/com/node/test/parallel/test-readline-set-raw-mode.js:90:8)atModule._compile(module.js:569:30)atObject.Module._extensions..js(module.js:580:10)atModule.load(module.js:503:32)attryModuleLoad(module.js:466:12)atFunction.Module._load(module.js:458:3)atFunction.Module.runMain(module.js:605:10)atstartup(bootstrap_node.js:144:16)atbootstrap_node.js:561:3Command: out/Release/node/Users/gib/wrk/com/node/test/parallel/test-readline-set-raw-mode.js[01:42|%100|+1563|-2]: Done

That assertion was added in nodejs/node-v0.x-archive#3757, which fixed nodejs/node-v0.x-archive#3756.

[gibfahn]

Changed the test to assert that there is 1 listener until you close, and 0 listeners afterwards. Not sure if that's the intended behaviour.

Also not sure whether this would be semver-major or semver-patch, I think this is just a bug, but 🤷‍♂️ .

CI: https://ci.nodejs.org/job/node-test-commit/10208/

[jasnell]

Landed in 7d53aba

[addaleax]

@gibfahn This breaks real-world applications: #13557

We should probably revert for now?

[jasnell]

I'm good with reverting but it would definitely be good to understand why it breaks things because the change is obstensibly correct ... There should not be any more data events after the close event, and there shouldn't be reason to keep the listener there. Preferably the reason for the break can be fixed so that this commit can be reapplied.

[gibfahn]

I'm good with reverting but it would definitely be good to understand why it breaks things...

For anyone reading this issue (possibly in the distant future 🛰 ), @addaleax has since answered this in #13560 (comment):

The interface instance passed to emitKeypressEvents() is explicitly not for decoder lifetime control; it is just used to toggle tab completion on it. We provide emitKeypressEvents() as a standalone method as well, which should work on its own exactly as documented (i.e. independently of any readline interfaces by default).

[MylesBorins]

I'm marking this as dont-land as it appears to be reverted. Let me know if this is a mistake