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

readline: line emitted after close while processing text file #22615

Closed
lych77 opened this issue Aug 31, 2018 · 3 comments · Fixed by #22679
Closed

readline: line emitted after close while processing text file #22615

lych77 opened this issue Aug 31, 2018 · 3 comments · Fixed by #22679

Comments

@lych77
Copy link

lych77 commented Aug 31, 2018

  • Version: v10.9.0
  • Platform: Windows 7 64-bit

Test code:

'use strict';

const fs = require('fs');
const readline = require('readline');

let sampleText = '';
for (let i = 0; i < 10; i++) {
	sampleText += `This is line ${i}\n`;
}
fs.writeFileSync('sample.txt', sampleText);

const file = fs.createReadStream('sample.txt', {encoding: 'utf-8'});
const reader = readline.createInterface(file);

reader.on('line', ln => {
	console.log(ln);
	if (ln.endsWith('4')) {
		reader.close();
	}
});

reader.on('close', () => {
	console.log('closed');
});

Output:

This is line 0
This is line 1
This is line 2
This is line 3
This is line 4
closed
This is line 5
This is line 6
This is line 7
This is line 8
This is line 9

Should this be supposed to happen? What should I do if I want to cancel the reading process under some condition? Is it the only way to maintain some flag myself and check it in the 'line' handler and just ignore the overheads?

@cjihrig
Copy link
Contributor

cjihrig commented Sep 2, 2018

The issue here is that the readable stream (file) emits the entire file in a single 'data' event. Each 'data' event queues up 0 or more 'line' events. So, by the time close() relinquishes control over the stream, all of the 'line' events are already queued up.

A few things that could be done:

  • track state yourself (as mentioned by OP)
  • Set a low value of highWaterMark for file.
  • Monkey patch Interface.prototype._onLine() (probably shouldn't do that though).
  • Core could do something, but this seems better off in userland code.

@lych77
Copy link
Author

lych77 commented Sep 3, 2018

Thanks, I've got the picture. Yes there are workarouds, I can also remove the 'line' listener as well when closing the interface. I might just be interested in if this is an intended behavior, or is considered as a problem too by the official team (no matter how late to fix it, as it's so trivial).

@cjihrig
Copy link
Contributor

cjihrig commented Sep 3, 2018

IMO, this is intended behavior. There is already a note in the pause() docs that hints at it. I've opened #22679 to add a similar warning to close().

@lych77 lych77 closed this as completed Sep 4, 2018
cjihrig added a commit to cjihrig/node that referenced this issue Sep 7, 2018
When close() is called on a readline instance, it is possible
that data is already buffered, and will trigger 'line' events.
This commit adds a warning to the corresponding docs. Note that
a similar warning already exists for the pause() method.

PR-URL: nodejs#22679
Fixes: nodejs#22615
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this issue Sep 7, 2018
When close() is called on a readline instance, it is possible
that data is already buffered, and will trigger 'line' events.
This commit adds a warning to the corresponding docs. Note that
a similar warning already exists for the pause() method.

PR-URL: #22679
Fixes: #22615
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants