Add autoClose=true option to fs.createWrireStream #20880 #25275
Conversation
| @@ -1760,7 +1761,11 @@ function WriteStream(path, options) { | |||
| this.open(); | |||
|
|
|||
| // dispose on finish. | |||
| this.once('finish', this.close); | |||
| this.once('finish', function(){ | |||
|
@cjihrig thanks for reviewing, apologies for space problems, i'll fix them and upload updated code. |
|
@cjihrig thank you very much for comments, i did the respective code changes |
|
/cc @joyent/node-coreteam |
| @@ -1742,6 +1742,13 @@ function WriteStream(path, options) { | |||
| this.mode = options.hasOwnProperty('mode') ? options.mode : 438; /*=0666*/ | |||
|
|
|||
| this.start = options.hasOwnProperty('start') ? options.start : undefined; | |||
|
|
|||
| if (options.hasOwnProperty('autoClose')) { | |||
There was a problem hiding this comment.
For consistency with the lines above it, this should probably use a ternary operator.
There was a problem hiding this comment.
Actually at first i used ternary operator but i got lint error 'line too long' something so i removed ternary.
There was a problem hiding this comment.
Will this be fine?
this.autoClose = options.hasOwnProperty('autoClose') ?
options.autoClose : true;
There was a problem hiding this comment.
With make lint below code works fine
this.autoClose = options.hasOwnProperty('autoClose') ?
options.autoClose : true;
|
@cjihrig Thanks for comments, I have updated all comments. Please have a look. |
|
Looks like you missed these two. https://github.com/joyent/node/pull/25275/files#r30064983 |
|
@cjihrig I have updated all comments. Please have a look. |
|
LGTM cc: @joyent/node-coreteam |
|
@cjihrig Shouldn't we land this in master instead of v0.12? See our guidelines for landing changes in patch releases:
EDIT: It looks like I suggested to submit the PR against v0.12, so that was my mistake, I apologize for the confusion. Landing it on master should not require any change though. |
| next(); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Minor style issue: unnecessary blank line.
| assert(!stream.fd); | ||
| }); | ||
| }); | ||
| } |
|
@saquibkhan The commits will need to be squashed into just one commit before this can be merged. Also, the commit message will need to be adjusted to conform to the contributing guidelines. If something in these guidelines is not clear, please join #libuv on Freenode and we'll be able to help you. Thank you again 👍 |
|
@misterdjules Thanks You for your comments, i have updated the code. I wonder that lint doesn't run on test files , should it catch space issues in test file too :-) |
|
@saquibkhan I don't know why If you have some time to make the change to include them and see how much work it would be to fix the style for all tests, that would be very much appreciated! |
|
@misterdjules Sure I would look into it why make jshint misses test files and also fix the issue. |
|
@misterdjules Do we need to raise an issue for fixing |
|
LGTM, @jasnell @joyent/node-coreteam I would like to land this in master, would that interfere with the ongoing work in https://github.com/jasnell/node.js-convergence? |
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 === true finishes, it is not destroyed. Its underlying fd is not closed and it is the responsibility of the user to close it. Fixes nodejs#20880
|
@misterdjules ... sorry, I didn't see your previous ping to me on this. This would need to land over in nodejs/node master now. |
|
@jasnell @misterdjules How this is going to land on nodejs/node? Do i need to raise a new pull request on nodejs/node ? |
|
@saquibkhan Yes, the best way to move this forward is to submit a new PR at nodejs/node targeted to the master branch. Thank you again! |
|
+1. If the changes also need to land in v5/v4/v0.12/v0.10, please indicate so in the new PR and we can get the PR labeled and ensure things are properly backported. Feel free to cc @misterdjules and I on the PR so we can track! |
Adding autoClose=true/false option to fs.createWrireStream.