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

fs.read at position 0 followed by position null reads wrong data #8397

Closed
defunctzombie opened this issue Sep 2, 2016 · 11 comments
Closed
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@defunctzombie
Copy link
Contributor

  • Version: 6.4.0
  • Platform: Ubuntu 14.04 x64
  • Subsystem: fs

After reading from position 0 (using fs.read), subsequence read with position null reads the incorrect data.

I would expect a read from position 0 followed by a read from null position to read from the next set of data per the documentation:

position is an integer specifying where to begin reading from in the file. If position is null, data will be read from the current file position.

Save the following to a file and run it.

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

// change this to toggle failing vs passing behavior
const fail = true;

fs.open(__filename, 'r', (err, fd) => {
    assert(!err);

    const buff = new Buffer(4);

    // this is the line that causes incorrect (unexpected?) behavior
    // if 0 is passed to read, then the subsequent read with 'null'
    // results in the wrong data being read
    // when `null` is used I get `cons` followed by `t fs`
    // when 0 is used first, I get `cons` followed by `cons`
    fs.read(fd, buff, 0, 4, (fail) ? 0 : null, (err, bytes, buffer) => {
        assert(!err);
        assert.equal(buffer.toString(), 'cons');
        console.log(buffer.toString());
        fs.read(fd, buff, 0, 4, null, (err, bytes, buffer) => {
            assert(!err);
            assert.equal(buffer.toString(), 't fs');
            console.log(buffer.toString());
        });
    });
});
@Fishrock123 Fishrock123 added the fs Issues and PRs related to the fs subsystem / file system. label Sep 3, 2016
@mscdex
Copy link
Contributor

mscdex commented Sep 3, 2016

What's happening here is that when you specify a position >= 0, pread() is used which does not modify the file pointer. When you pass a null position, read() is used which starts from the current file pointer.

@defunctzombie
Copy link
Contributor Author

Should the docs be updated to make this clear? Or would it be useful to
make the core implementation track the position internally and behave
appropriately when using a null read after a positional read?

On Friday, September 2, 2016, Brian White [email protected] wrote:

What's happening here is that when you specify a position >= 0, pread()
is used which does not modify the file pointer. When you pass a null
position, then the read occurs from the current file pointer.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#8397 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFLOKuSFGzvWXi7ZcI7Brjdfq31b3xRks5qmMmwgaJpZM4J0K7-
.

@mscdex
Copy link
Contributor

mscdex commented Sep 3, 2016

/cc @nodejs/collaborators

@bnoordhuis
Copy link
Member

Should the docs be updated to make this clear?

The documentation for fs.read() seems clear enough to me:

`position` is an integer specifying where to begin reading from in the file.
If `position` is `null`, data will be read from the current file position.

Or would it be useful to make the core implementation track the position internally and behave appropriately when using a null read after a positional read?

Node.js has worked this way since forever so I don't think we would want to change that. It's covered by fs.createReadStream(path, { start: position }).

@mscdex
Copy link
Contributor

mscdex commented Sep 3, 2016

I should also note that if you need/want seeking support (to set the file pointer), there are always addons like fs-ext which provide fs.seek()/fs.seekSync(). Functions like that might be nice to have in core finally and should work on both *nix and Windows.

@bnoordhuis
Copy link
Member

fs.seek()/fs.seekSync()

Those are really bad anti-patterns when reading or writing asynchronously because there is no consistent ordering of events. Consider:

fs.read(...);
fs.seekSync(...);

Does the seek happen before or after the read? No one knows. No one can know.

@mscdex
Copy link
Contributor

mscdex commented Sep 3, 2016

@bnoordhuis Sure but if you call fs.seekSync() within the fs.read() callback (and before the next fs.read() in the original example), then it's not a problem.

What happens when you do:

fs.read(fd, buf1, 0, 4, null, ...);
fs.read(fd, buf2, 0, 4, null, ...);

? Is there a defined order in that scenario?

@bnoordhuis
Copy link
Member

There isn't, you should use positional reads in that case.

@defunctzombie
Copy link
Contributor Author

The documentation for fs.read() seems clear enough to me:

Disagree. It is unclear for the original example I posted. If you do a read starting at position 0, you then think (rightfully so because nothing tells you otherwise) that the file position is now 0 + N bytes (4 in my example) and that a read using null will read from where the last read ended.

There isn't, you should use positional reads in that case.

I don't disagree that this is the better strategy (to be explicit in your read positions in these cases), which is why I think the documentation should make it clearer and guide the developer in understanding that position specified reading is disjoint from null position reading.

@Trott
Copy link
Member

Trott commented Jul 10, 2017

Documentation improvement PR would be welcome.

/cc @nodejs/documentation

If no one already reading this is inclined to submit a PR, then perhaps it makes sense to put a good first contribution label on the issue.

@dcharbonnier
Copy link
Contributor

for me something has changed on 8.2 compared to 8.1, the documentation is confusing and #8397 (comment) is correct

Trott pushed a commit to Trott/io.js that referenced this issue Aug 4, 2017
What happen to the file position after a read using a position null or
integer was not clear and you can assume that the cursor of the file
descriptor is updated even if position is an integer.

Fixes: nodejs#8397
@Trott Trott closed this as completed in 680285c Aug 7, 2017
addaleax pushed a commit that referenced this issue Aug 10, 2017
What happen to the file position after a read using a position null or
integer was not clear and you can assume that the cursor of the file
descriptor is updated even if position is an integer.

PR-URL: #14631
Fixes: #8397
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 20, 2017
What happen to the file position after a read using a position null or
integer was not clear and you can assume that the cursor of the file
descriptor is updated even if position is an integer.

PR-URL: #14631
Fixes: #8397
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
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

Successfully merging a pull request may close this issue.

6 participants