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

Node read stream hangs on specific file, base64 encoding #19862

Closed
jan-j opened this issue Apr 7, 2018 · 9 comments
Closed

Node read stream hangs on specific file, base64 encoding #19862

jan-j opened this issue Apr 7, 2018 · 9 comments
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.

Comments

@jan-j
Copy link

jan-j commented Apr 7, 2018

  • Version: 7.9.0/8.9.3/9.7.1
  • Platform: Windows/Linux/MacOS

I have following code working for every file except the one that keeps hanging without emitting end or error events (I tried other stream events too).

const fs = require('fs');

const rs = fs.createReadStream(filePath, {
    encoding: 'base64',
});

rs.on('data', () => {
    console.log('data');
});
rs.on('end', () => {
    console.log('end');
});
rs.on('error', e => {
    console.log('error', e);
});

If I move read point with start option to 1 instead of 0 it works properly. Same if highWaterMark is set to value other than default. It doesn't really help as it seems it can fail with other "corrupted" file.

It seems like Node bug, but maybe there's something I'm missing here.

Here's file to recreate the issue:
http://s3.eu-west-1.amazonaws.com/jjapitest/file

Here's interactive demo of the issue:
https://repl.it/repls/AnimatedDisguisedNumerator

@addaleax addaleax added confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. v6.x labels Apr 7, 2018
@addaleax
Copy link
Member

addaleax commented Apr 7, 2018

Fwiw, this seems to work on master but not on the other release lines, so maybe it’s just about figuring out which change fixed this.

@marcosc90
Copy link
Contributor

marcosc90 commented Jun 1, 2018

It will fail If highWaterMark is file.length - 1, and encoding base64, at least on v8.x.

In the following snippet, not even data is emitted.

const fs = require('fs');
const invalid = '/tmp/invalid';
fs.writeFileSync(invalid, '1234');

function readFileBase64(filePath) {
  const rs = fs.createReadStream(filePath, {
      encoding: 'base64',
      highWaterMark: 2 // with 3 data is called, but end no
  });

  rs.on('data', (data) => {
      console.log(filePath, 'data', Buffer.from(data, 'base64').toString());
      // Nothing for hwm == 2
      // 123 for hwm == 3
      // 123 & 4 for hwm == 4 == file.length
  });
  rs.on('end', () => {
      console.log(filePath, 'end');
  });
  rs.on('error', e => {
      console.log(filePath, 'error', e);
  });
}

readFileBase64(invalid);

This works fine on v10.1.0

I'm willing to track down the changes on master to fix this issue.

@marcosc90
Copy link
Contributor

marcosc90 commented Jun 2, 2018

@addaleax I found the issue, and already solved it for v8.x. I haven't got the time to go through all the code in _stream_readable.js, so if someone with more expertise in the subject care to take a look at my code, to make sure nothing else would break, even though all tests are passing, that would be great.

@mcollina
Copy link
Member

mcollina commented Jun 2, 2018

cc @nodejs/streams

@mcollina
Copy link
Member

mcollina commented Jun 2, 2018

I fear this might not be fixable in Node 6, 8, 9 without backporting a semver-major change: #18994

@ChALkeR ChALkeR added v9.x and removed v9.x labels Jun 30, 2018
@jasnell jasnell removed the v9.x label Aug 12, 2018
@antsmartian
Copy link
Contributor

Are we planning to backport?

@mcollina
Copy link
Member

@antsmartian I would not recommend it, as it could actually break things.

@mcollina
Copy link
Member

This is fixed in Node 10, I'm closing this.

@antsmartian
Copy link
Contributor

Thanks for the update @mcollina

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

7 participants