{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/node/issues/9699","repository_url":"https://api.github.com/repos/nodejs/node","labels_url":"https://api.github.com/repos/nodejs/node/issues/9699/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/node/issues/9699/comments","events_url":"https://api.github.com/repos/nodejs/node/issues/9699/events","html_url":"https://github.com/nodejs/node/pull/9699","id":190509611,"number":9699,"title":"fs: make sure readFile[Sync] reads from the beginning","user":{"login":"seishun","id":988441,"avatar_url":"https://avatars.githubusercontent.com/u/988441?v=3","gravatar_id":"","url":"https://api.github.com/users/seishun","html_url":"https://github.com/seishun","followers_url":"https://api.github.com/users/seishun/followers","following_url":"https://api.github.com/users/seishun/following{/other_user}","gists_url":"https://api.github.com/users/seishun/gists{/gist_id}","starred_url":"https://api.github.com/users/seishun/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/seishun/subscriptions","organizations_url":"https://api.github.com/users/seishun/orgs","repos_url":"https://api.github.com/users/seishun/repos","events_url":"https://api.github.com/users/seishun/events{/privacy}","received_events_url":"https://api.github.com/users/seishun/received_events","type":"User","site_admin":false},"labels":[{"id":155435785,"url":"https://api.github.com/repos/nodejs/node/labels/fs","name":"fs","color":"0052cc","default":false},{"id":169485514,"url":"https://api.github.com/repos/nodejs/node/labels/semver-major","name":"semver-major","color":"fbca04","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2016-11-19T17:47:22Z","updated_at":"2016-12-06T15:14:01Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/nodejs/node/pulls/9699","html_url":"https://github.com/nodejs/node/pull/9699","diff_url":"https://github.com/nodejs/node/pull/9699.diff","patch_url":"https://github.com/nodejs/node/pull/9699.patch"},"body":"<!--\r\nThank you for your pull request. Please review below requirements.\r\n\r\nBug fixes and new features should include tests and possibly benchmarks.\r\n\r\nContributors guide: https://github.com/nodejs/node/blob/master/CONTRIBUTING.md\r\n-->\r\n\r\n##### Checklist\r\n<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->\r\n\r\n- [x] `make -j8 test` (UNIX), or `vcbuild test nosign` (Windows) passes\r\n- [ ] tests and/or benchmarks are included\r\n- [x] commit message follows commit guidelines\r\n\r\n##### Affected core subsystem(s)\r\n<!-- Provide affected core subsystem(s) (like doc, cluster, crypto, etc). -->\r\nfs\r\n\r\n##### Description of change\r\n<!-- Provide a description of the change below this comment. -->\r\n\r\nCurrently, `fs.readFile[Sync]` starts reading from the current file position, which can be non-zero if the `fd` was previously read or written from/to. This conflicts with the [documentation](https://nodejs.org/api/fs.html#fs_fs_readfile_file_options_callback) which states that it \"reads the entire contents of a file\".\r\n\r\nFixes: https://github.com/nodejs/node/issues/9671\r\n\r\nProvisional testcase for `fs.readFile`:\r\n\r\n```js\r\nconst assert = require('assert');\r\nconst fs = require('fs');\r\n\r\nconst filleLength = 2;\r\nconst path = 'test-read-file';\r\nconst fd = fs.openSync(path, 'w+');\r\n\r\nfs.writeSync(fd, Buffer.alloc(filleLength, 1), 0, filleLength);\r\n\r\nfs.readFile(fd, (err, buf) => {\r\n  if (err) throw err;\r\n  assert.strictEqual(buf.length, filleLength);\r\n});\r\n```\r\n\r\nFor `fs.readFileSync`:\r\n\r\n```js\r\n'use strict';\r\n\r\nconst assert = require('assert');\r\nconst fs = require('fs');\r\n\r\nconst fileLength = 2;\r\nconst path = 'test-read-file';\r\nconst fd = fs.openSync(path, 'w+');\r\n\r\nfs.writeSync(fd, Buffer.alloc(fileLength, 1), 0, fileLength);\r\n\r\nassert.strictEqual(fs.readFileSync(fd).length, fileLength);\r\n```"},"comment":{"url":"https://api.github.com/repos/nodejs/node/issues/comments/265175050","html_url":"https://github.com/nodejs/node/pull/9699#issuecomment-265175050","issue_url":"https://api.github.com/repos/nodejs/node/issues/9699","id":265175050,"user":{"login":"sam-github","id":17607,"avatar_url":"https://avatars.githubusercontent.com/u/17607?v=3","gravatar_id":"","url":"https://api.github.com/users/sam-github","html_url":"https://github.com/sam-github","followers_url":"https://api.github.com/users/sam-github/followers","following_url":"https://api.github.com/users/sam-github/following{/other_user}","gists_url":"https://api.github.com/users/sam-github/gists{/gist_id}","starred_url":"https://api.github.com/users/sam-github/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sam-github/subscriptions","organizations_url":"https://api.github.com/users/sam-github/orgs","repos_url":"https://api.github.com/users/sam-github/repos","events_url":"https://api.github.com/users/sam-github/events{/privacy}","received_events_url":"https://api.github.com/users/sam-github/received_events","type":"User","site_admin":false},"created_at":"2016-12-06T15:14:01Z","updated_at":"2016-12-06T15:14:01Z","body":"> @sam-github I think the test should test just one thing, namely that doing readFile[Sync] repeatedly returns the same data every time.\r\n\r\n@seishun Is this test not intended to test the API contract of readFile? If so, the API contract is not that it returns the same data every time, but that it returns the entire *file contents* every time - the test as written doesn't do that. Calling stat will at least guarantee that what you read from disk is the same size as what is on disk. Right now if a bug was introduced so that only the first block of a data was returned when reading from a fd, your test looks like it would pass. To be more thorough, you could readFile, change the data on disk, and readFile again... but since that isn't a suspected regression its overkill at this moment."}}
{"actor":{"display_login":"sam-github"}}