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

Undocumented and possibly wrong behavior of historySize in readline #6336

Closed
vsemozhetbyt opened this issue Apr 21, 2016 · 0 comments
Closed
Labels
readline Issues and PRs related to the built-in readline module.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 21, 2016

  • Subsystem: readline

This is all the docs say about historySize in readline:

historySize - maximum number of history lines retained. Defaults to 30.

If I want to use readline for reading a big file line by line (see example in docs), I don't need the history tracking. However I cannot set historySize to 0:

console.log(
  require('readline').createInterface({
    input: require('fs').createReadStream(__filename, 'utf8'),
    historySize: 0
  }).historySize
);

gives 30;

If I get it right, it is due to this fragment in the readline.js:

historySize = historySize || kHistorySize;

I don't know if failure to distinguish 0 and undefined here is a bug or a feature.

Maybe these fixes could be done:

Tactically:

  1. Clarify in the docs that 0 is not a legal value for historySize.
  2. May be add a throw in the readline.js (currently the function throws only if historySize < 0). Now the user value is silently secretly discarded.

Strategically:

  1. Add the possibility to turn off history tracking by setting historySize to 0. I test some code with historySize: 30 and historySize: 1 — the second case uses slightly less memory, but slightly more time. The history size has less impact on the performance than history tracking itself because of all the operations in the _addHistory function. If I want to read the file with 100000 lines, the 100000 bunches of array operations in this function is a considerable write-off overhead.
@addaleax addaleax added the readline Issues and PRs related to the built-in readline module. label Apr 21, 2016
suryagh added a commit to suryagh/node that referenced this issue Apr 22, 2016
1.  the historySize to default to 30 only if undefined.
2.  if historySize is set to 0, then disable caching the line.
3. Added unit tests
suryagh added a commit to suryagh/node that referenced this issue Apr 22, 2016
suryagh added a commit to suryagh/node that referenced this issue Apr 25, 2016
fix nodejs#6336
1.  The `historySize` to default to `30` only if `undefined`.
2.  If `historySize` is set to 0, then disable caching the line.
3.  Added unit tests.
4.  Updated documentation.
jasnell pushed a commit that referenced this issue Apr 26, 2016
1.  The `historySize` to default to `30` only if `undefined`.
2.  If `historySize` is set to 0, then disable caching the line.
3.  Added unit tests.
4.  Updated documentation.

Fixes: #6336
PR-URL: #6352
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants