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 sometimes produces lines with \r in them #45992

Closed
alecmev opened this issue Dec 28, 2022 · 5 comments · Fixed by #46306
Closed

readline sometimes produces lines with \r in them #45992

alecmev opened this issue Dec 28, 2022 · 5 comments · Fixed by #46306
Labels
readline Issues and PRs related to the built-in readline module.

Comments

@alecmev
Copy link
Contributor

alecmev commented Dec 28, 2022

Version

19.3.0

Platform

6.1.1-arch1-1 x86_64 GNU/Linux

Subsystem

readline

What steps will reproduce the bug?

Unfortunately, the files readline has trouble with are proprietary and I can't share them. I have some ideas on how to craft one, but it would take some time, so I'd like to see first if it will be needed at all.

How often does it reproduce? Is there a required condition?

Consistently. Isn't present in 19.2.0 and earlier.

What is the expected behavior?

No \r in any of the lines.

What do you see instead?

Some of the files (not all) produce 1-3 lines with an \r in it. In one file it happens 21870 lines in (109243 preceding characters), in another 75432, in another 120396. I see no pattern, but it's deterministic.

Additional information

Here are a few lines from one of the files:

  0
VERTEX
  8
14
 10
22.0936
 20
12.9141
  0
VERTEX
  8
14
 10
22.8017
 20
12.4141
  0
VERTEX
  8
14
 10
23.6255
 20
11.9128
  0

The lines are \r\n-delimited. Could be caused by #45614? Not sure why only a couple of lines get affected.

@VoltrexKeyva VoltrexKeyva added the readline Issues and PRs related to the built-in readline module. label Dec 28, 2022
@aduh95
Copy link
Contributor

aduh95 commented Dec 28, 2022

Do you set crlfDelay option? The recommended value for files is Infinity as shown in the doc example. FWIW if you use filehandle.readLines(), you won't have to worry about this.

@alecmev
Copy link
Contributor Author

alecmev commented Dec 29, 2022

Thanks for looking into this! I do have crlfDelay set to Infinity. I'm using createInterface because I don't want to restrict the library to just files. The source is a simple createReadStream.

@aduh95
Copy link
Contributor

aduh95 commented Dec 29, 2022

Can you open a PR to add a failing test case to test/known_issues? Or if you’re able to fix the issue yourself that’s even better of course, but having a test would already be a big help :)

alecmev added a commit to alecmev/node that referenced this issue Jan 3, 2023
@alecmev
Copy link
Contributor Author

alecmev commented Jan 3, 2023

Failing test added: #46075

alecmev added a commit to alecmev/node that referenced this issue Jan 4, 2023
aduh95 pushed a commit that referenced this issue Jan 22, 2023
PR-URL: #46075
Refs: #45992
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Jan 22, 2023

Interestingly, it "just" works if the input file has 0x3333 lines, but fails as soon as it has 0x3334 or more. I'm investigating.

The lines are each 5-byte long in the test case, and 0x3333*5 = 0xffff. If I change the test to use 4-byte long lines, it no longer reproduce. My guess is that having a carriage return on position 0xffff is problematic. 0xffff = (64 * 1024) - 1, and 64*1024 is the default highWaterMark according to https://nodejs.org/api/fs.html#fscreatereadstreampath-options.

By using fs.createReadStream(filepath, { highWaterMark: 6 }), I'm able to reproduce the issue from the first second line.

EDIT: I've simplified the test case to be minimal, and opened #46306 with a fix.

aduh95 added a commit to aduh95/node that referenced this issue Jan 22, 2023
nodejs-github-bot pushed a commit that referenced this issue Jan 24, 2023
Fixes: #45992
PR-URL: #46306
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
ruyadorno pushed a commit that referenced this issue Feb 1, 2023
PR-URL: #46075
Refs: #45992
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
ruyadorno pushed a commit that referenced this issue Feb 1, 2023
Fixes: #45992
PR-URL: #46306
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 3, 2023
PR-URL: #46075
Refs: #45992
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 3, 2023
Fixes: #45992
PR-URL: #46306
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 5, 2023
PR-URL: #46075
Refs: #45992
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
juanarbol pushed a commit that referenced this issue Mar 5, 2023
Fixes: #45992
PR-URL: #46306
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yagiz Nizipli <[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.

3 participants