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

[Bug report] fs.createReadStream set highWaterMark, the same file get different result #35465

Closed
Zclhlmgqzc opened this issue Oct 2, 2020 · 6 comments
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@Zclhlmgqzc
Copy link

Zclhlmgqzc commented Oct 2, 2020

What steps will reproduce the bug?

test-202.js

const { createReadStream } = require('fs')

// const highWaterMark = 127
// ./test-file-200
// 23
// ./test-file-200
// 23

// const highWaterMark = 128
// ./test-file-200
// 20
// ./test-file-200
// 20

// const highWaterMark = 129
// ./test-file-202
// 17
// ./test-file-202
// 128
// 18

// const highWaterMark = 160
// ./test-file-202
// 84
// ./test-file-202
// 84

// const highWaterMark = 199
// ./test-file-202
// 6
// ./test-file-202
// 184
// 21

const highWaterMark = 202

async function readFile(inputPath) {
  console.log(inputPath)

  const readStream = createReadStream(inputPath, {
    encoding: 'binary',
    highWaterMark
  })

  for await (const chunk of readStream) {
    if (chunk.length !== highWaterMark) {
      console.log(chunk.length)
    }
  }
}

async function test() {
  for (const file of ['./test-file-202', './test-file-202']) {
    await readFile(file)
  }
}

test()

test-file-202
(10 * 20 + /r/n)

 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789
 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789

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

Always with node v12.2.0-v12.18.4-14.13.0 and highWaterMark > 128
Works fine in node v10.22.1 v12.1.0 or highWaterMark <= 128

What is the expected behavior?

the same result

What do you see instead?

node test-202
./test-file-202
./test-file-202
200
2

Additional information

v12.2.0
fs: align fs.ReadStream buffer pool writes to 8-byte boundary
#24838

@Zclhlmgqzc
Copy link
Author

Zclhlmgqzc commented Oct 5, 2020

@bnoordhuis
@trxcllnt

when bytesRead === 0 why poolFragments.push(thisPool.slice(alignedStart, alignedEnd));

node-v12.2.0/lib/internal/fs/streams.js

ReadStream.prototype._read = function(n) {
  if (typeof this.fd !== 'number') {
    return this.once('open', function() {
      this._read(n);
    });
  }

  if (this.destroyed)
    return;

  if (!pool || pool.length - pool.used < kMinPoolSpace) {
    // Discard the old pool.
    allocNewPool(this.readableHighWaterMark);
  }

  // Grab another reference to the pool in the case that while we're
  // in the thread pool another read() finishes up the pool, and
  // allocates a new one.
  const thisPool = pool;
  let toRead = Math.min(pool.length - pool.used, n);
  const start = pool.used;

  if (this.pos !== undefined)
    toRead = Math.min(this.end - this.pos + 1, toRead);
  else
    toRead = Math.min(this.end - this.bytesRead + 1, toRead);

  // Already read everything we were supposed to read!
  // treat as EOF.
  if (toRead <= 0)
    return this.push(null);

  // the actual read.
  fs.read(this.fd, pool, pool.used, toRead, this.pos, (er, bytesRead) => {
    if (er) {
      if (this.autoClose) {
        this.destroy();
      }
      this.emit('error', er);
    } else {
      let b = null;
      // Now that we know how much data we have actually read, re-wind the
      // 'used' field if we can, and otherwise allow the remainder of our
      // reservation to be used as a new pool later.
      if (start + toRead === thisPool.used && thisPool === pool) {
        console.log('newUsed')
        const newUsed = thisPool.used + bytesRead - toRead;
        thisPool.used = roundUpToMultipleOf8(newUsed);
      } else {
        console.log('alignedEnd')
        console.log('bytesRead: ' + bytesRead)
        // Round down to the next lowest multiple of 8 to ensure the new pool
        // fragment start and end positions are aligned to an 8 byte boundary.
        const alignedEnd = (start + toRead) & ~7;
        const alignedStart = roundUpToMultipleOf8(start + bytesRead);
        if (alignedEnd - alignedStart >= kMinPoolSpace) {
          console.log('start: ' + start)
          console.log('toRead: ' + toRead)
          console.log('alignedEnd: ' + alignedEnd)
          console.log('alignedStart: ' + alignedStart)
          poolFragments.push(thisPool.slice(alignedStart, alignedEnd));
        }
      }

      if (bytesRead > 0) {
        this.bytesRead += bytesRead;
        b = thisPool.slice(start, start + bytesRead);
      }

      this.push(b);
    }
  });
node test-202.js 
./test-file-202
alignedEnd
bytesRead: 202

alignedEnd
bytesRead: 202

alignedEnd
bytesRead: 0
start: 0
toRead: 202
alignedEnd: 200
alignedStart: 0

./test-file-202

newUsed

200

alignedEnd
bytesRead: 202

alignedEnd
bytesRead: 2
start: 0
toRead: 202
alignedEnd: 200
alignedStart: 8

2
newUsed

@Zclhlmgqzc
Copy link
Author

Zclhlmgqzc commented Oct 6, 2020

Works fine
node-12.2.0/12.18.4/14.13.0
/lib/internal/fs/streams.js

      if (start + toRead === thisPool.used && thisPool === pool) {
        const newUsed = thisPool.used + bytesRead - toRead;
        thisPool.used = roundUpToMultipleOf8(newUsed);
    - } else {
    + } else if (bytesRead > 0) {

@watilde watilde added the fs Issues and PRs related to the fs subsystem / file system. label Oct 10, 2020
@ronag
Copy link
Member

ronag commented Oct 10, 2020

@Zclhlmgqzc PR welcome! Do you have the same issue on Node 15/master?

@Zclhlmgqzc
Copy link
Author

Zclhlmgqzc commented Oct 14, 2020

@ronag
Works fine in Node 15.x
Because #33981

@targos targos added the v14.x label Dec 13, 2020
@gblock0
Copy link

gblock0 commented Mar 23, 2021

Hi, is there an update on this? Do you think it'll make it into 12.x or 14.x? I don't have a ton of extra time, but if you give me some direction, I can take a swing at fixing it.

@targos
Copy link
Member

targos commented May 3, 2023

Closing as v14.x is EoL.

@targos targos closed this as not planned Won't fix, can't repro, duplicate, stale May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests

5 participants