From 75e035571bccd475206f9104d1c377ae359ccd4b Mon Sep 17 00:00:00 2001 From: meixg Date: Fri, 25 Mar 2022 17:25:14 +0800 Subject: [PATCH 1/9] readline: fix question still called after closed resolve: https://github.com/nodejs/node/issues/42450 --- lib/readline.js | 1 + test/parallel/test-readline-interface.js | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/lib/readline.js b/lib/readline.js index 11514614c33ab9..183c3141529a5b 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -127,6 +127,7 @@ const superQuestion = _Interface.prototype.question; * @returns {void} */ Interface.prototype.question = function(query, options, cb) { + if (this.closed) return; cb = typeof options === 'function' ? options : cb; options = typeof options === 'object' && options !== null ? options : {}; diff --git a/test/parallel/test-readline-interface.js b/test/parallel/test-readline-interface.js index ad7eee7c42e485..5ef016ea356a0f 100644 --- a/test/parallel/test-readline-interface.js +++ b/test/parallel/test-readline-interface.js @@ -1092,6 +1092,18 @@ for (let i = 0; i < 12; i++) { rli.close(); } + // Call question after close + { + const [rli, fi] = getInterface({ terminal }); + rli.question('What\'s your name?', common.mustCall((name) => { + assert.strictEqual(name, 'Node.js'); + rli.close(); + rli.question('How are you?', common.mustNotCall()); + assert.notStrictEqual(rli.getPrompt(), 'How are you?'); + })); + fi.emit('data', 'Node.js\n'); + } + // Can create a new readline Interface with a null output argument { const [rli, fi] = getInterface({ output: null, terminal }); From 0e4bd7b492b2a31725f2340381d26fb2314c4114 Mon Sep 17 00:00:00 2001 From: meixg Date: Fri, 25 Mar 2022 18:36:02 +0800 Subject: [PATCH 2/9] readline: fix question still called after closed handle promises version --- lib/readline/promises.js | 5 +++++ test/parallel/test-readline-promises-interface.js | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/lib/readline/promises.js b/lib/readline/promises.js index 534558ec31ffdc..56b8fc9b70b0af 100644 --- a/lib/readline/promises.js +++ b/lib/readline/promises.js @@ -27,6 +27,11 @@ class Interface extends _Interface { return new Promise((resolve, reject) => { let cb = resolve; + if (this.closed) { + cb(); + return; + } + if (options?.signal) { validateAbortSignal(options.signal, 'options.signal'); if (options.signal.aborted) { diff --git a/test/parallel/test-readline-promises-interface.js b/test/parallel/test-readline-promises-interface.js index cf0543d7d255f7..0dec27da72979a 100644 --- a/test/parallel/test-readline-promises-interface.js +++ b/test/parallel/test-readline-promises-interface.js @@ -952,6 +952,21 @@ for (let i = 0; i < 12; i++) { rli.close(); } + // Call question after close + { + const [rli, fi] = getInterface({ terminal }); + rli.question('What\'s your name?').then(common.mustCall((name) => { + assert.strictEqual(name, 'Node.js'); + rli.close(); + rli.question('How are you?').then(common.mustCall((ans) => { + assert.strictEqual(ans, undefined); + })); + assert.notStrictEqual(rli.getPrompt(), 'How are you?'); + })); + fi.emit('data', 'Node.js\n'); + } + + // Can create a new readline Interface with a null output argument { const [rli, fi] = getInterface({ output: null, terminal }); From 75c38f3a63e84f30fd1dcd3fbc6304a7ec3b0367 Mon Sep 17 00:00:00 2001 From: meixg Date: Fri, 25 Mar 2022 21:20:10 +0800 Subject: [PATCH 3/9] readline: fix question still called after closed let the promise in a never settling state and make the change in the super class --- lib/internal/readline/interface.js | 1 + lib/readline.js | 1 - lib/readline/promises.js | 5 ----- test/parallel/test-readline-promises-interface.js | 4 +--- 4 files changed, 2 insertions(+), 9 deletions(-) diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index b5a5455ed3da85..c0a34a29cfe5ef 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -398,6 +398,7 @@ class Interface extends InterfaceConstructor { } question(query, cb) { + if (this.closed) return; if (this[kQuestionCallback]) { this.prompt(); } else { diff --git a/lib/readline.js b/lib/readline.js index 183c3141529a5b..11514614c33ab9 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -127,7 +127,6 @@ const superQuestion = _Interface.prototype.question; * @returns {void} */ Interface.prototype.question = function(query, options, cb) { - if (this.closed) return; cb = typeof options === 'function' ? options : cb; options = typeof options === 'object' && options !== null ? options : {}; diff --git a/lib/readline/promises.js b/lib/readline/promises.js index 56b8fc9b70b0af..534558ec31ffdc 100644 --- a/lib/readline/promises.js +++ b/lib/readline/promises.js @@ -27,11 +27,6 @@ class Interface extends _Interface { return new Promise((resolve, reject) => { let cb = resolve; - if (this.closed) { - cb(); - return; - } - if (options?.signal) { validateAbortSignal(options.signal, 'options.signal'); if (options.signal.aborted) { diff --git a/test/parallel/test-readline-promises-interface.js b/test/parallel/test-readline-promises-interface.js index 0dec27da72979a..367a303e9615c3 100644 --- a/test/parallel/test-readline-promises-interface.js +++ b/test/parallel/test-readline-promises-interface.js @@ -958,9 +958,7 @@ for (let i = 0; i < 12; i++) { rli.question('What\'s your name?').then(common.mustCall((name) => { assert.strictEqual(name, 'Node.js'); rli.close(); - rli.question('How are you?').then(common.mustCall((ans) => { - assert.strictEqual(ans, undefined); - })); + rli.question('How are you?').then(common.mustNotCall()); assert.notStrictEqual(rli.getPrompt(), 'How are you?'); })); fi.emit('data', 'Node.js\n'); From e57fe919c5890a5bba8ea00836e3f07d2fda6801 Mon Sep 17 00:00:00 2001 From: meixg Date: Fri, 25 Mar 2022 21:33:05 +0800 Subject: [PATCH 4/9] readline: fix question still called after closed add promisified question test case --- test/parallel/test-readline-interface.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/parallel/test-readline-interface.js b/test/parallel/test-readline-interface.js index 5ef016ea356a0f..b048f3eb32779a 100644 --- a/test/parallel/test-readline-interface.js +++ b/test/parallel/test-readline-interface.js @@ -1104,6 +1104,19 @@ for (let i = 0; i < 12; i++) { fi.emit('data', 'Node.js\n'); } + // Call promisified question after close + { + const [rli, fi] = getInterface({ terminal }); + const question = util.promisify(rli.question).bind(rli); + question('What\'s your name?').then(common.mustCall((name) => { + assert.strictEqual(name, 'Node.js'); + rli.close(); + question('How are you?').then(common.mustNotCall()); + assert.notStrictEqual(rli.getPrompt(), 'How are you?'); + })); + fi.emit('data', 'Node.js\n'); + } + // Can create a new readline Interface with a null output argument { const [rli, fi] = getInterface({ output: null, terminal }); From 88088f46c4c74049c2f9556f54c37bd6e65c9a5d Mon Sep 17 00:00:00 2001 From: meixg Date: Fri, 25 Mar 2022 21:35:56 +0800 Subject: [PATCH 5/9] readline: fix question still called after closed change doc --- doc/api/readline.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/doc/api/readline.md b/doc/api/readline.md index 58a67256104b24..d9c0c36c4fa4ef 100644 --- a/doc/api/readline.md +++ b/doc/api/readline.md @@ -330,6 +330,9 @@ The `callback` function passed to `rl.question()` does not follow the typical pattern of accepting an `Error` object or `null` as the first argument. The `callback` is called with the provided answer as the only argument. +If called after `rl.close()`, `rl.question()` will do nothing and the +`callback` function passed to `rl.question()` will not be called. + Example usage: ```js @@ -586,6 +589,9 @@ paused. If the `readlinePromises.Interface` was created with `output` set to `null` or `undefined` the `query` is not written. +If the question is called after `rl.close()`, it returns a promise in a never +settling state. + Example usage: ```mjs @@ -855,6 +861,9 @@ The `callback` function passed to `rl.question()` does not follow the typical pattern of accepting an `Error` object or `null` as the first argument. The `callback` is called with the provided answer as the only argument. +If called after `rl.close()`, `rl.question()` will do nothing and the +`callback` function passed to `rl.question()` will not be called. + Example usage: ```js From ee388bdcb042f876cabff70c120633855783fec8 Mon Sep 17 00:00:00 2001 From: meixg Date: Mon, 28 Mar 2022 13:31:09 +0800 Subject: [PATCH 6/9] readline: fix question still called after closed throw an error --- doc/api/errors.md | 8 ++++++++ doc/api/readline.md | 9 +++------ lib/internal/errors.js | 1 + lib/internal/readline/interface.js | 9 +++++++-- test/parallel/test-readline-interface.js | 14 ++++++++++++-- test/parallel/test-readline-promises-interface.js | 7 ++++++- 6 files changed, 37 insertions(+), 11 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index a11a94981fefa6..4066f24cfe50e6 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2365,6 +2365,14 @@ Accessing `Object.prototype.__proto__` has been forbidden using [`Object.setPrototypeOf`][] should be used to get and set the prototype of an object. + + +### `ERR_READLINE_CLOSED` + +> Stability: 1 - Experimental + +An attempt was made to use a readline that was already closed. + ### `ERR_REQUIRE_ESM` diff --git a/doc/api/readline.md b/doc/api/readline.md index d9c0c36c4fa4ef..d3a01755fa0f57 100644 --- a/doc/api/readline.md +++ b/doc/api/readline.md @@ -330,8 +330,7 @@ The `callback` function passed to `rl.question()` does not follow the typical pattern of accepting an `Error` object or `null` as the first argument. The `callback` is called with the provided answer as the only argument. -If called after `rl.close()`, `rl.question()` will do nothing and the -`callback` function passed to `rl.question()` will not be called. +An error will be thrown if calling `rl.question()` after `rl.close()`. Example usage: @@ -589,8 +588,7 @@ paused. If the `readlinePromises.Interface` was created with `output` set to `null` or `undefined` the `query` is not written. -If the question is called after `rl.close()`, it returns a promise in a never -settling state. +If the question is called after `rl.close()`, it returns a rejected promise. Example usage: @@ -861,8 +859,7 @@ The `callback` function passed to `rl.question()` does not follow the typical pattern of accepting an `Error` object or `null` as the first argument. The `callback` is called with the provided answer as the only argument. -If called after `rl.close()`, `rl.question()` will do nothing and the -`callback` function passed to `rl.question()` will not be called. +An error will be thrown if calling `rl.question()` after `rl.close()`. Example usage: diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 5f75c0290b33d9..47fe69c2d72e74 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1475,6 +1475,7 @@ E('ERR_PACKAGE_PATH_NOT_EXPORTED', (pkgPath, subpath, base = undefined) => { E('ERR_PERFORMANCE_INVALID_TIMESTAMP', '%d is not a valid timestamp', TypeError); E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError); +E('ERR_READLINE_CLOSED', 'Readline was closed', Error); E('ERR_REQUIRE_ESM', function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) { hideInternalStackFrames(this); diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index c0a34a29cfe5ef..3b5687bec01ee9 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -38,7 +38,10 @@ const { const { codes } = require('internal/errors'); -const { ERR_INVALID_ARG_VALUE } = codes; +const { + ERR_INVALID_ARG_VALUE, + ERR_READLINE_CLOSED, +} = codes; const { validateAbortSignal, validateArray, @@ -398,7 +401,9 @@ class Interface extends InterfaceConstructor { } question(query, cb) { - if (this.closed) return; + if (this.closed) { + throw new ERR_READLINE_CLOSED(); + } if (this[kQuestionCallback]) { this.prompt(); } else { diff --git a/test/parallel/test-readline-interface.js b/test/parallel/test-readline-interface.js index b048f3eb32779a..810ecdf3dc1959 100644 --- a/test/parallel/test-readline-interface.js +++ b/test/parallel/test-readline-interface.js @@ -1098,7 +1098,12 @@ for (let i = 0; i < 12; i++) { rli.question('What\'s your name?', common.mustCall((name) => { assert.strictEqual(name, 'Node.js'); rli.close(); - rli.question('How are you?', common.mustNotCall()); + assert.throws(() => { + rli.question('How are you?', common.mustNotCall()); + }, { + name: 'Error', + code: 'ERR_READLINE_CLOSED' + }); assert.notStrictEqual(rli.getPrompt(), 'How are you?'); })); fi.emit('data', 'Node.js\n'); @@ -1111,7 +1116,12 @@ for (let i = 0; i < 12; i++) { question('What\'s your name?').then(common.mustCall((name) => { assert.strictEqual(name, 'Node.js'); rli.close(); - question('How are you?').then(common.mustNotCall()); + question('How are you?') + .then(common.mustNotCall()) + .catch(common.expectsError({ + code: 'ERR_READLINE_CLOSED', + name: 'Error' + })); assert.notStrictEqual(rli.getPrompt(), 'How are you?'); })); fi.emit('data', 'Node.js\n'); diff --git a/test/parallel/test-readline-promises-interface.js b/test/parallel/test-readline-promises-interface.js index 367a303e9615c3..86f3aff2f2ef3f 100644 --- a/test/parallel/test-readline-promises-interface.js +++ b/test/parallel/test-readline-promises-interface.js @@ -958,7 +958,12 @@ for (let i = 0; i < 12; i++) { rli.question('What\'s your name?').then(common.mustCall((name) => { assert.strictEqual(name, 'Node.js'); rli.close(); - rli.question('How are you?').then(common.mustNotCall()); + rli.question('How are you?') + .then(common.mustNotCall()) + .catch(common.expectsError({ + code: 'ERR_READLINE_CLOSED', + name: 'Error' + })); assert.notStrictEqual(rli.getPrompt(), 'How are you?'); })); fi.emit('data', 'Node.js\n'); From 6db86863d6fa3a11a8d7ded2fb75725d2177d494 Mon Sep 17 00:00:00 2001 From: meixg Date: Wed, 6 Apr 2022 10:54:49 +0800 Subject: [PATCH 7/9] readline: fix question still called after closed change according to review --- test/parallel/test-readline-interface.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/parallel/test-readline-interface.js b/test/parallel/test-readline-interface.js index 810ecdf3dc1959..b382fb77d6cf89 100644 --- a/test/parallel/test-readline-interface.js +++ b/test/parallel/test-readline-interface.js @@ -1117,8 +1117,7 @@ for (let i = 0; i < 12; i++) { assert.strictEqual(name, 'Node.js'); rli.close(); question('How are you?') - .then(common.mustNotCall()) - .catch(common.expectsError({ + .then(common.mustNotCall(), common.expectsError({ code: 'ERR_READLINE_CLOSED', name: 'Error' })); From cb80a3ad22dd2f4c5241cb21923aadc53176d250 Mon Sep 17 00:00:00 2001 From: meixg Date: Wed, 6 Apr 2022 11:10:25 +0800 Subject: [PATCH 8/9] readline: fix question still called after closed change to a more generically error code name --- doc/api/errors.md | 16 ++++++++-------- lib/internal/errors.js | 2 +- lib/internal/readline/interface.js | 4 ++-- test/parallel/test-readline-interface.js | 4 ++-- .../parallel/test-readline-promises-interface.js | 2 +- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 4066f24cfe50e6..71cae6300b26ed 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2365,14 +2365,6 @@ Accessing `Object.prototype.__proto__` has been forbidden using [`Object.setPrototypeOf`][] should be used to get and set the prototype of an object. - - -### `ERR_READLINE_CLOSED` - -> Stability: 1 - Experimental - -An attempt was made to use a readline that was already closed. - ### `ERR_REQUIRE_ESM` @@ -2810,6 +2802,14 @@ import 'package-name'; // supported `import` with URL schemes other than `file` and `data` is unsupported. + + +### `ERR_USE_AFTER_CLOSE` + +> Stability: 1 - Experimental + +An attempt was made to use something that was already closed. + ### `ERR_VALID_PERFORMANCE_ENTRY_TYPE` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 47fe69c2d72e74..a5c64080a59aae 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1475,7 +1475,6 @@ E('ERR_PACKAGE_PATH_NOT_EXPORTED', (pkgPath, subpath, base = undefined) => { E('ERR_PERFORMANCE_INVALID_TIMESTAMP', '%d is not a valid timestamp', TypeError); E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError); -E('ERR_READLINE_CLOSED', 'Readline was closed', Error); E('ERR_REQUIRE_ESM', function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) { hideInternalStackFrames(this); @@ -1628,6 +1627,7 @@ E('ERR_UNSUPPORTED_ESM_URL_SCHEME', (url, supported) => { msg += `. Received protocol '${url.protocol}'`; return msg; }, Error); +E('ERR_USE_AFTER_CLOSE', '%s was closed', Error); // This should probably be a `TypeError`. E('ERR_VALID_PERFORMANCE_ENTRY_TYPE', diff --git a/lib/internal/readline/interface.js b/lib/internal/readline/interface.js index 3b5687bec01ee9..47e5ca580ab317 100644 --- a/lib/internal/readline/interface.js +++ b/lib/internal/readline/interface.js @@ -40,7 +40,7 @@ const { codes } = require('internal/errors'); const { ERR_INVALID_ARG_VALUE, - ERR_READLINE_CLOSED, + ERR_USE_AFTER_CLOSE, } = codes; const { validateAbortSignal, @@ -402,7 +402,7 @@ class Interface extends InterfaceConstructor { question(query, cb) { if (this.closed) { - throw new ERR_READLINE_CLOSED(); + throw new ERR_USE_AFTER_CLOSE('readline'); } if (this[kQuestionCallback]) { this.prompt(); diff --git a/test/parallel/test-readline-interface.js b/test/parallel/test-readline-interface.js index b382fb77d6cf89..52cfdac2b8f88a 100644 --- a/test/parallel/test-readline-interface.js +++ b/test/parallel/test-readline-interface.js @@ -1102,7 +1102,7 @@ for (let i = 0; i < 12; i++) { rli.question('How are you?', common.mustNotCall()); }, { name: 'Error', - code: 'ERR_READLINE_CLOSED' + code: 'ERR_USE_AFTER_CLOSE' }); assert.notStrictEqual(rli.getPrompt(), 'How are you?'); })); @@ -1118,7 +1118,7 @@ for (let i = 0; i < 12; i++) { rli.close(); question('How are you?') .then(common.mustNotCall(), common.expectsError({ - code: 'ERR_READLINE_CLOSED', + code: 'ERR_USE_AFTER_CLOSE', name: 'Error' })); assert.notStrictEqual(rli.getPrompt(), 'How are you?'); diff --git a/test/parallel/test-readline-promises-interface.js b/test/parallel/test-readline-promises-interface.js index 86f3aff2f2ef3f..8e0532bd254717 100644 --- a/test/parallel/test-readline-promises-interface.js +++ b/test/parallel/test-readline-promises-interface.js @@ -961,7 +961,7 @@ for (let i = 0; i < 12; i++) { rli.question('How are you?') .then(common.mustNotCall()) .catch(common.expectsError({ - code: 'ERR_READLINE_CLOSED', + code: 'ERR_USE_AFTER_CLOSE', name: 'Error' })); assert.notStrictEqual(rli.getPrompt(), 'How are you?'); From ac7c84644de3161ea10ccbb01dfd4add4ad991ae Mon Sep 17 00:00:00 2001 From: meixg Date: Fri, 8 Apr 2022 10:52:20 +0800 Subject: [PATCH 9/9] readline: fix question still called after closed change according to cr --- test/parallel/test-readline-promises-interface.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/parallel/test-readline-promises-interface.js b/test/parallel/test-readline-promises-interface.js index 8e0532bd254717..455dabe6749bd5 100644 --- a/test/parallel/test-readline-promises-interface.js +++ b/test/parallel/test-readline-promises-interface.js @@ -959,8 +959,7 @@ for (let i = 0; i < 12; i++) { assert.strictEqual(name, 'Node.js'); rli.close(); rli.question('How are you?') - .then(common.mustNotCall()) - .catch(common.expectsError({ + .then(common.mustNotCall(), common.expectsError({ code: 'ERR_USE_AFTER_CLOSE', name: 'Error' }));