Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception thrown in readline.createInterface with input stream #30885

Closed
ww9rivers opened this issue Dec 10, 2019 · 8 comments
Closed

Exception thrown in readline.createInterface with input stream #30885

ww9rivers opened this issue Dec 10, 2019 · 8 comments
Labels
readline Issues and PRs related to the built-in readline module. stalled Issues and PRs that are stalled.

Comments

@ww9rivers
Copy link

I have a piece of code that reads a text file directly from a .tar.gz archive. The code started to throw an exception with Node v13.3.0.

Debugging the code, I found that, when the execution reached this point (line 446 in readline.js):

Interface.prototype._normalWrite = function(b) {
  if (b === undefined) {
    return;
  }
  let string = this._decoder.write(b); // <<<<
  if (this._sawReturnAt &&
      DateNow() - this._sawReturnAt <= this.crlfDelay) {
    string = string.replace(/^\n/, '');
    this._sawReturnAt = 0;
  }

the this._decoder was undefined.

That function was invoked when a ondata handler was set on line 223:

  if (!this.terminal) {
    function onSelfCloseWithoutTerminal() {
      input.removeListener('data', ondata);
      input.removeListener('end', onend);
    }

    input.on('data', ondata);
    input.on('end', onend);
    self.once('close', onSelfCloseWithoutTerminal);
    this._decoder = new StringDecoder('utf8');
  } else {

It seems that the line of code that sets this._decoder should be moved up 3 lines.

Does that make sense?

@ww9rivers ww9rivers changed the title Exception thrown readline.createInterface with input stream Exception thrown in readline.createInterface with input stream Dec 10, 2019
@BridgeAR
Copy link
Member

@ww9rivers would you be so kind and post a minimal code piece with which it's possible to trigger the error?
It would also be great if you could check in what Node.js version the behavior changed. I can not find anything in the v13.3.0 release that should have impacted this, so I guess it either never worked or it broke in a former version.

@BridgeAR BridgeAR added the readline Issues and PRs related to the built-in readline module. label Dec 10, 2019
@ww9rivers
Copy link
Author

ww9rivers commented Dec 10, 2019

@ww9rivers would you be so kind and post a minimal code piece with which it's possible to trigger the error?

I have just put that in a gist:
https://gist.github.com/ww9rivers/5cd5343951f2ac0e966bf6c19c5f96ee

Thank you for the quick response! I was previously using a version 10 release, I believe. But I am not sure what the exact version was.

Edit: I edited the gist and add an archive file below:
tarball.tar.gz

Wonder if the exception has something to do with the use of Promise. I created another test, which is in the tarball, with Promise removed. That works OK.

@mscdex
Copy link
Contributor

mscdex commented Dec 11, 2019

It's better to post the code here instead of an external link in case the external link dies at some point.

FWIW here is the code:

const fs = require('fs');
const readline = require('readline');
const util = require('util');
const tar = require('tar');
const zlib = require('zlib');
const test = require("tap").test;


function get_nsconf (archive, nsconf) {
        return new Promise((resolve, reject) => {
                if (!nsconf) { nsconf = '/nsconfig/ns.conf' }
                console.debug('----> Parsing NetScaler backup: '+archive);
                fs.createReadStream(archive).on('error', console.error)
                        .pipe(zlib.createUnzip())
                        .pipe(new tar.Parse())
                        .on('entry', entry => {
                                if (entry.path.endsWith(nsconf)) {
                                        if ('File' !== entry.type) {
                                                throw new Error('Wrong type for entry: '+nsconf+' : '+entry.type);
                                        }
                                        resolve({tar: archive, entry: entry});
                                } else {
                                        entry.resume();
                                }
                        })
        });
}

test('NetScaler config read test', t => {
        const archive = './tarball.tar.gz';
        const nsconf = 'tap-tar.js';
        get_nsconf(archive, nsconf).then(nsc => {
                let count = 0;
                // processing nsc as resolved by get_nsconf():
                console.debug('----> Found: '+nsc.entry.path);
                t.ok(nsc.tar == archive);
                t.ok(nsc.entry != undefined);
                readline.createInterface({
                        input: nsc.entry
                }).on('line', line => {
                        count++
                        console.log(line);
                        t.ok(null !== line && undefined !== line);
                }).on('close', () => {
                        console.log('----> File closed. Line count = '+count);
                        t.end();
                });
                console.debug('----> Closing file. Line count = '+count);
        }).catch(err => {
                console.log('Error: ', err, '\n', util.inspect(err.stack));
        });
});

@ww9rivers
Copy link
Author

Thanks! I think I have two problems in the test above: The use of Promise is one. But the question I asked in the initial issue report is still valid.

So I created 3 tests, all in the tarball attached below. The tap-read.js is basically the same as the test posted above, which throws an error when run with Node v12.13.1. The other 2 tests work.

I cloned Node source and made the changed I suggested in the initial post above, tap-read.js still does not work but no longer throws an error either.
tarball.tar.gz

@BridgeAR
Copy link
Member

@ww9rivers would you be so kind and reduce your test case to Node.js core code only? Right now there are still multiple node modules involved. It should be possible to see if it is indeed a Node.js bug or not.

@antsmartian
Copy link
Contributor

@ww9rivers Could you please provide us a sample with only Node.js core code involved as @BridgeAR mentioned. That will help us to provide more information about the issue.

@RedYetiDev RedYetiDev closed this as not planned Won't fix, can't repro, duplicate, stale Jul 11, 2024
@RedYetiDev RedYetiDev added the stalled Issues and PRs that are stalled. label Jul 11, 2024
@RedYetiDev
Copy link
Member

@ww9rivers Could you please provide us a sample with only Node.js core code involved as @BridgeAR mentioned. That will help us to provide more information about the issue.

This comment was never addressed

Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module. stalled Issues and PRs that are stalled.
Projects
None yet
Development

No branches or pull requests

5 participants