fs: add autoClose option to fs.createWriteStream by saquibkhan · Pull Request #3679 · nodejs/node

saquibkhan · 2015-11-05T15:07:31Z

Add support to fs.createWriteStream and fs.WriteStream for an autoClose
option that behaves similarly to the autoClose option supported by
fs.createReadStream and fs.ReadStream.

When an instance of fs.WriteStream created with autoClose === false finishes,
it is not destroyed. Its underlying fd is not closed and it is the
responsibility of the user to close it.

saquibkhan · 2015-11-05T15:11:40Z

@jasnell @misterdjules As discussed I created a new pull request for autoClose feature. node-v0.x-archive/pull/25275

This PR can be landed on v5/v4/v0.12/v0.10

it would be really nice if i see this PR land :-)

Fishrock123 · 2015-11-05T15:15:17Z

cc @nodejs/streams

bnoordhuis · 2015-11-05T16:39:49Z

Can you use const for imports?

bnoordhuis · 2015-11-05T16:45:52Z

Basic premise looks alright to me. The checks in the test can be made a little tighter.

saquibkhan · 2015-11-05T18:22:39Z

@bnoordhuis Done! I updated the review comments given by you.

jasnell · 2015-11-05T18:30:21Z

Overall this is fine, but as a semver-minor it woud not be able to land in v0.10, v0.12 or v4.x.

jasnell · 2015-11-05T18:31:54Z

Would like to see this reworded a bit to make it absolutely obvious that the default behavior is autoClose = true. Perhaps move the third sentence to the front?

jasnell · 2015-11-05T18:34:08Z

LGTM with one nit on the docs

ronkorving · 2015-11-06T02:59:10Z

In your PR message, you write When an instance of fs.WriteStream created with autoClose === true finishes,, but I think you mean to say === false?

saquibkhan · 2015-11-06T03:24:05Z

@jasnell @ronkorving Done! Updated the doc and commit message. Thanks!

ronkorving · 2015-11-06T03:29:43Z

The PR message still says "true" instead of "false", sorry :) Commit message is 👍

saquibkhan · 2015-11-06T16:30:40Z

@ronkorving Done!

bnoordhuis · 2015-11-06T17:53:56Z

Indent errors.

jasnell · 2015-11-13T20:30:32Z

@saquibkhan ... when you get a chance, please take a look at @bnoordhuis' comments, then please rebase and update :-)

saquibkhan · 2015-11-14T17:55:50Z

I am using ! to test fd is not null

if you're specifically testing for not null, perhaps assert(stream.fd !== null) would be better

saquibkhan · 2015-11-14T17:56:57Z

@bnoordhuis @jasnell I have updated the code as per review comments.

ronkorving · 2015-11-16T03:40:02Z

@saquibkhan "This branch has conflicts that must be resolved"

cjihrig · 2015-11-30T17:07:47Z

Can you add a !! on the last options.autoClose to guarantee we have a Boolean.

saquibkhan · 2015-12-10T15:38:04Z

@cjihrig @ronkorving @jasnell @bnoordhuis Updated the code as per the review comments.. LGTM

cjihrig · 2015-12-10T17:25:42Z

LGTM pending CI

Add support to fs.createWriteStream and fs.createWriteStream for an autoClose option that behaves similarly to the autoClose option supported by fs.createReadStream and fs.ReadStream. When an instance of fs.createWriteStream created with autoClose === false finishes, it is not destroyed. Its underlying fd is not closed and it is the responsibility of the user to close it.

saquibkhan · 2016-01-11T04:45:15Z

@ronkorving @jasnell @bnoordhuis @cjihrig Can we merge?

jasnell · 2016-01-11T17:56:09Z

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

jasnell · 2016-01-11T19:00:51Z

unrelated failure in CI

Add support to fs.createWriteStream and fs.createWriteStream for an autoClose option that behaves similarly to the autoClose option supported by fs.createReadStream and fs.ReadStream. When an instance of fs.createWriteStream created with autoClose === false finishes, it is not destroyed. Its underlying fd is not closed and it is the responsibility of the user to close it. PR-URL: #3679 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2016-01-11T19:05:54Z

Landed in 6039a7c

Add support to fs.createWriteStream and fs.createWriteStream for an autoClose option that behaves similarly to the autoClose option supported by fs.createReadStream and fs.ReadStream. When an instance of fs.createWriteStream created with autoClose === false finishes, it is not destroyed. Its underlying fd is not closed and it is the responsibility of the user to close it. PR-URL: #3679 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: * events: make sure console functions exist (Dave) #4479 * fs: add autoClose option to fs.createWriteStream (Saquib) #3679 * http: improves expect header handling (Daniel Sellers) #4501 * node: allow preload modules with -i (Evan Lucas) #4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) #4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622 - module: cache stat() results more aggressively (Ben Noordhuis) #4575 - querystring: improve parse() performance (Brian White) #4675 PR-URL: #4742

Add support to fs.createWriteStream and fs.createWriteStream for an autoClose option that behaves similarly to the autoClose option supported by fs.createReadStream and fs.ReadStream. When an instance of fs.createWriteStream created with autoClose === false finishes, it is not destroyed. Its underlying fd is not closed and it is the responsibility of the user to close it. PR-URL: nodejs#3679 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Notable changes: * events: make sure console functions exist (Dave) nodejs#4479 * fs: add autoClose option to fs.createWriteStream (Saquib) nodejs#3679 * http: improves expect header handling (Daniel Sellers) nodejs#4501 * node: allow preload modules with -i (Evan Lucas) nodejs#4696 * v8,src: expose statistics about heap spaces (`v8.getHeapSpaceStatistics()`) (Ben Ripkens) nodejs#4463 * Minor performance improvements: - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - module: cache stat() results more aggressively (Ben Noordhuis) nodejs#4575 - querystring: improve parse() performance (Brian White) nodejs#4675 PR-URL: nodejs#4742

Fishrock123 added fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 5, 2015

bnoordhuis reviewed Nov 5, 2015
View reviewed changes

Comment threadtest/parallel/test-fs-write-stream-autoclose-option.js Outdated
bnoordhuisNov 5, 2015
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use const for imports?

saquibkhan force-pushed the feature-autoclose branch from 4a6f713 to 991b6f5Compare November 5, 2015 18:21

saquibkhan force-pushed the feature-autoclose branch from 991b6f5 to 4400ee7Compare November 5, 2015 18:25

jasnell reviewed Nov 5, 2015
View reviewed changes

saquibkhan force-pushed the feature-autoclose branch from 4400ee7 to 273c758Compare November 6, 2015 03:22

bnoordhuis reviewed Nov 6, 2015
View reviewed changes

Comment threadtest/parallel/test-fs-write-stream-autoclose-option.js Outdated
bnoordhuisNov 6, 2015
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent errors.

saquibkhan force-pushed the feature-autoclose branch from 273c758 to c82c2a9Compare November 14, 2015 17:54

saquibkhan reviewed Nov 14, 2015
View reviewed changes

cjihrig reviewed Nov 30, 2015
View reviewed changes

Comment threadlib/fs.js Outdated
cjihrigNov 30, 2015
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a !! on the last options.autoClose to guarantee we have a Boolean.

saquibkhan force-pushed the feature-autoclose branch 2 times, most recently from 3b9ffce to 3f24d5cCompare December 10, 2015 15:36

saquibkhan changed the title ~~fs: add autoClose option to fs.WriteStream~~fs: add autoClose option to fs.createWriteStreamDec 11, 2015

saquibkhan force-pushed the feature-autoclose branch from 3f24d5c to 98849bcCompare December 11, 2015 10:24

Trott force-pushed the master branch from 1e896a6 to 082cc8dCompare December 27, 2015 02:00

jasnell closed this Jan 11, 2016

evanlucas mentioned this pull request Jan 18, 2016
v5.5.0 Release Proposal (Stable) #4742
Merged

Uh oh!

Uh oh!

Conversation

saquibkhan commented Nov 5, 2015

Uh oh!

saquibkhan commented Nov 5, 2015

Uh oh!

Fishrock123 commented Nov 5, 2015

Uh oh!

bnoordhuisNov 5, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Nov 5, 2015

Uh oh!

saquibkhan commented Nov 5, 2015

Uh oh!

jasnell commented Nov 5, 2015

Uh oh!

jasnellNov 5, 2015

Choose a reason for hiding this comment

Uh oh!

jasnell commented Nov 5, 2015

Uh oh!

ronkorving commented Nov 6, 2015

Uh oh!

saquibkhan commented Nov 6, 2015

Uh oh!

ronkorving commented Nov 6, 2015

Uh oh!

saquibkhan commented Nov 6, 2015

Uh oh!

bnoordhuisNov 6, 2015

Choose a reason for hiding this comment

Uh oh!

jasnell commented Nov 13, 2015

Uh oh!

saquibkhanNov 14, 2015

Choose a reason for hiding this comment

Uh oh!

jasnellNov 16, 2015

Choose a reason for hiding this comment

Uh oh!

saquibkhan commented Nov 14, 2015

Uh oh!

ronkorving commented Nov 16, 2015

Uh oh!

cjihrigNov 30, 2015

Choose a reason for hiding this comment

Uh oh!

saquibkhan commented Dec 10, 2015

Uh oh!

cjihrig commented Dec 10, 2015

Uh oh!

saquibkhan commented Jan 11, 2016

Uh oh!

jasnell commented Jan 11, 2016

Uh oh!

jasnell commented Jan 11, 2016

Uh oh!

jasnell commented Jan 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants