stream: the position of _read() is wrong by helloyou2012 · Pull Request #38292 · nodejs/node

helloyou2012 · 2021-04-19T10:05:00Z

VoltrexKeyva · 2021-04-19T10:46:17Z

Hi @helloyou2012, could you remove the merge commit from your branch? Merge commits tend to break the tooling, you can use git rebase instead.

node/doc/guides/contributing/pull-requests.md

Lines 215 to 217 in 54322b8

    
           As a best practice, once you have committed your changes, it is a good idea 
        
           to use `git rebase` (not `git merge`) to synchronize your work with the main 
        
           repository.

benjamingr · 2021-04-19T12:44:39Z

@nodejs/streams

mcollina

Why is wrong? Can you please expand?

Can you please add a unit test?

ronag

Needs test and also a little unsure what exactly we are solving here. If I would guess:

Currently this.pos could actually be larger than the file size, which is a little weird.
Currently this.pos is what we have requested, but necessarily what we have read.

helloyou2012 · 2021-04-19T15:28:42Z

@mcollina see this issue: #33940, this

node/lib/internal/streams/readable.js

Line 487 in d4f33f1

this._read(state.highWaterMark);

will call _read(n) multiple times with n= highWaterMark, but the bytesRead may be not equal to n, then the data will loss. Example:

// file data is: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]
// pos = 0;
_read(10)
// if bytesRead = 3 then buf = [0, 1, 2]
// pos = 10
_read(10)
// if bytesRead = 10 then buf = [10, 11, 12], so [3, 4, 5, 6, 7, 8, 9] is loss.

Ayase-252 · 2021-04-19T16:15:21Z

The problem arised because current implementation assumed next position to start reading is this.pos + n

node/lib/internal/fs/streams.js

Lines 275 to 277 in d4f33f1

    
           if (this.pos !== undefined) { 
        
             this.pos += n; 
        
           }

But in reality, it does not necessarily read n bytes from file. In the callback of file handle read, it is possible that bytesRead does not equal n (less than n in my exprimentation)

node/lib/internal/fs/streams.js

Line 246 in d4f33f1

.read(this.fd, buf, 0, n, this.pos, (er, bytesRead, buf) => {

As a result, some bytes in file may be skipped. The "assumed" reading is longer than the "actual". pos moves too far.

About the test, the issue occurred because read in the above line reads less bytes bytesRead than requested n. How could we reproduce (or mock) such condition in test?

ronag

I think this change could make sense. But we need a test.

benjamingr · 2021-04-19T19:55:02Z

About the test, the issue occurred because read in the above line reads less bytes bytesRead than requested n. How could we reproduce (or mock) such condition in test?

Good question. I'm not sure I don't think I ever wrote such a test in Node -

A good way to try is to take the test case from #33940 and to try to make a smaller isolated case from it (shorter interval and smaller watermark)

@nodejs/fs

helloyou2012 · 2021-04-20T03:51:28Z

I added test case. @ronag @benjamingr

nodejs-github-bot · 2021-04-20T07:18:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37573/

benjamingr · 2021-04-20T12:19:20Z

test/parallel/test-fs-read-stream-pos.js

+    highWaterMark: hwm,
+    start: cur
+  });
+  stream.on('data', common.mustCallAtLeast((chunk) => {


Any reason this is a mustCAllAtLeast and not a mustCall?

mustCall is exact times, but this place will call at least 1 time not exact 1 time.

test/parallel/test-fs-read-stream-pos.js

benjamingr

LGTM if the test passes after this and fails in master

benjamingr · 2021-04-20T12:20:00Z

Also thank you for the meaningful contribution and working with us 🙏

mcollina

lgtm, hopefully the setInterval test would not fail in CI.

ronag

LGTM. Was wondering why this hasn't landed yet. Turns out it's my fault.

nodejs-github-bot · 2021-04-25T19:16:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37713/

nodejs-github-bot · 2021-04-26T09:47:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37732/

nodejs-github-bot · 2021-04-26T18:56:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37743/

ronag · 2021-04-26T18:59:34Z

I think there is a problem on arm with the new test. @helloyou2012

helloyou2012 · 2021-04-27T11:17:06Z

I think there is a problem on arm with the new test. @helloyou2012

Yes, the test timed out. I think it is better to set a time after which to exit safely. I will fix later.

Fixes: nodejs#33940

jasnell · 2021-04-28T18:47:52Z

Removing the author ready label as there's still work to be done here.

nodejs-github-bot · 2021-05-01T21:29:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37862/

nodejs-github-bot · 2021-05-02T09:52:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37873/

ronag · 2021-05-02T09:52:51Z

@helloyou2012 this still seems to have problems passing. Could you try and look at the CI failures? I started another CI just in case.

nodejs-github-bot · 2021-05-02T11:12:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37875/

helloyou2012 · 2021-05-02T13:58:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37873/

It failed cause of inspector-cli/test-inspector-cli-pid timed out, not of this.

helloyou2012 · 2021-05-02T14:00:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37875/

This CI all passed.

mcollina · 2021-05-03T09:56:12Z

Landed in d826f6b

Fixes: #33940 PR-URL: #38292 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Apr 19, 2021

helloyou2012 mentioned this pull request Apr 19, 2021

readStream with highWaterMark causes 'data' event to miss data #33940

Closed

mcollina requested changes Apr 19, 2021

View reviewed changes

ronag requested changes Apr 19, 2021

View reviewed changes

helloyou2012 force-pushed the master branch from 40bc8f6 to 5c7fd50 Compare April 19, 2021 15:06

ronag reviewed Apr 19, 2021

View reviewed changes

helloyou2012 force-pushed the master branch from 5c7fd50 to 5faff30 Compare April 20, 2021 03:48

benjamingr reviewed Apr 20, 2021

View reviewed changes

test/parallel/test-fs-read-stream-pos.js Show resolved Hide resolved

benjamingr approved these changes Apr 20, 2021

View reviewed changes

helloyou2012 force-pushed the master branch from 5faff30 to 70e1ed4 Compare April 20, 2021 14:57

mcollina approved these changes Apr 20, 2021

View reviewed changes

ronag approved these changes Apr 25, 2021

View reviewed changes

ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Apr 25, 2021

stream: the position of _read() is wrong

3922e39

Fixes: nodejs#33940

helloyou2012 force-pushed the master branch from 70e1ed4 to 3922e39 Compare April 28, 2021 02:49

jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 28, 2021

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 2, 2021

mcollina closed this May 3, 2021

ronag added the lts-watch-v14.x label May 3, 2021

targos mentioned this pull request May 3, 2021

v16.1.0 release proposal #38522

Merged

targos mentioned this pull request Jun 6, 2021

v14.17.1 release proposal #38948

Merged

targos removed the lts-watch-v14.x label Sep 4, 2021

Uh oh!

Conversation

helloyou2012 commented Apr 19, 2021

Uh oh!

VoltrexKeyva commented Apr 19, 2021

Uh oh!

benjamingr commented Apr 19, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

helloyou2012 commented Apr 19, 2021

Uh oh!

Ayase-252 commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Apr 19, 2021

Uh oh!

helloyou2012 commented Apr 20, 2021

Uh oh!

nodejs-github-bot commented Apr 20, 2021

Uh oh!

benjamingr Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

helloyou2012 Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Apr 20, 2021

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 25, 2021

Uh oh!

nodejs-github-bot commented Apr 26, 2021

Uh oh!

nodejs-github-bot commented Apr 26, 2021

Uh oh!

ronag commented Apr 26, 2021

Uh oh!

helloyou2012 commented Apr 27, 2021

Uh oh!

jasnell commented Apr 28, 2021

Uh oh!

nodejs-github-bot commented May 1, 2021

Uh oh!

nodejs-github-bot commented May 2, 2021

Uh oh!

ronag commented May 2, 2021

Uh oh!

nodejs-github-bot commented May 2, 2021

Uh oh!

helloyou2012 commented May 2, 2021

Uh oh!

helloyou2012 commented May 2, 2021

Uh oh!

mcollina commented May 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Ayase-252 commented Apr 19, 2021 •

edited

Loading