fs: add autoClose option to fs.createWriteStream #3679

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

test/parallel/test-fs-write-stream-autoclose-option.js

+var common = require('../common');
+var assert = require('assert');
+var path = require('path');
+var fs = require('fs');


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

doc/api/fs.markdown

@@ -876,6 +877,12 @@ than replacing it may require a `flags` mode of `r+` rather than the
 default mode `w`. The `defaultEncoding` can be `'utf8'`, `'ascii'`, `binary`,
 or `'base64'`.

+If `autoClose` is false, then the file descriptor won't be closed, even if


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

test/parallel/test-fs-write-stream-autoclose-option.js

+    process.nextTick(function() {
+          assert.strictEqual(stream.closed, true);
+          assert.strictEqual(!stream.fd, true);
+        });


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

test/parallel/test-fs-write-stream-autoclose-option.js

+stream.on('finish', common.mustCall(function() {
+  process.nextTick(common.mustCall(function() {
+    assert.strictEqual(stream.closed, undefined);
+    assert.strictEqual(!stream.fd, false);


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

lib/fs.js

@@ -1886,6 +1886,7 @@ function WriteStream(path, options) {
  this.mode = options.mode === undefined ? 0o666 : options.mode;

  this.start = options.start;
+  this.autoClose = options.autoClose === undefined ? true : options.autoClose;


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

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

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

jasnell reviewed Nov 5, 2015
View reviewed changes

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

bnoordhuis reviewed Nov 6, 2015
View reviewed changes

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

saquibkhan reviewed Nov 14, 2015
View reviewed changes

cjihrig reviewed Nov 30, 2015
View reviewed changes

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

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

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

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: add autoClose option to fs.createWriteStream #3679

fs: add autoClose option to fs.createWriteStream #3679

saquibkhan commented Nov 5, 2015

saquibkhan commented Nov 5, 2015

Fishrock123 commented Nov 5, 2015

bnoordhuis Nov 5, 2015

bnoordhuis commented Nov 5, 2015

saquibkhan commented Nov 5, 2015

jasnell commented Nov 5, 2015

jasnell Nov 5, 2015

jasnell commented Nov 5, 2015

ronkorving commented Nov 6, 2015

saquibkhan commented Nov 6, 2015

ronkorving commented Nov 6, 2015

saquibkhan commented Nov 6, 2015

bnoordhuis Nov 6, 2015

jasnell commented Nov 13, 2015

saquibkhan Nov 14, 2015

jasnell Nov 16, 2015

saquibkhan commented Nov 14, 2015

ronkorving commented Nov 16, 2015

cjihrig Nov 30, 2015

saquibkhan commented Dec 10, 2015

cjihrig commented Dec 10, 2015

saquibkhan commented Jan 11, 2016

jasnell commented Jan 11, 2016

jasnell commented Jan 11, 2016

jasnell commented Jan 11, 2016

fs: add autoClose option to fs.createWriteStream #3679

fs: add autoClose option to fs.createWriteStream #3679

Conversation

saquibkhan commented Nov 5, 2015

saquibkhan commented Nov 5, 2015

Fishrock123 commented Nov 5, 2015

bnoordhuis Nov 5, 2015

Choose a reason for hiding this comment

bnoordhuis commented Nov 5, 2015

saquibkhan commented Nov 5, 2015

jasnell commented Nov 5, 2015

jasnell Nov 5, 2015

Choose a reason for hiding this comment

jasnell commented Nov 5, 2015

ronkorving commented Nov 6, 2015

saquibkhan commented Nov 6, 2015

ronkorving commented Nov 6, 2015

saquibkhan commented Nov 6, 2015

bnoordhuis Nov 6, 2015

Choose a reason for hiding this comment

jasnell commented Nov 13, 2015

saquibkhan Nov 14, 2015

Choose a reason for hiding this comment

jasnell Nov 16, 2015

Choose a reason for hiding this comment

saquibkhan commented Nov 14, 2015

ronkorving commented Nov 16, 2015

cjihrig Nov 30, 2015

Choose a reason for hiding this comment

saquibkhan commented Dec 10, 2015

cjihrig commented Dec 10, 2015

saquibkhan commented Jan 11, 2016

jasnell commented Jan 11, 2016

jasnell commented Jan 11, 2016

jasnell commented Jan 11, 2016