fs: replace a bind() with a top-level function #13474
Conversation
nodejs-github-bot
added
the
fs
|
cc @mscdex |
lib/fs.js
Outdated
| @@ -2034,6 +2034,11 @@ ReadStream.prototype.close = function(cb) { | |||
| this.fd = null; | |||
| }; | |||
|
|
|||
| // needed because as it will be called with arguments | |||
| // that does not match this.close() signature | |||
| function closeOnOpen() { | |||
There was a problem hiding this comment.
Maybe we can make it more evident by adding an fd parameter here? Plus I'm sure V8 would prefer it.
lib/fs.js
Outdated
| // needed because as it will be called with arguments | ||
| // that does not match this.close() signature | ||
| function closeOnOpen() { | ||
| this.close(); |
There was a problem hiding this comment.
yes, that's the exact meaning. this will be the stream object. I'll add a comment to note that.
There was a problem hiding this comment.
EventEmitter instances always call their event handlers within the context of the EventEmitter. I'm not sure we need to explicitly document that in this one particular place when we do this same thing throughout core.
There was a problem hiding this comment.
I agree with @mscdex. I looked at the code too quickly earlier.
|
LGTM |
|
@refack I've amended the commit message, check if it is ok and I'll merge. |
mcollina
changed the title
nodejs#11225 introduce an unnecessary bind() when closing a stream. This PR replaces that bind() with a top-level function. PR-URL: nodejs#13474 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Evan Lucas <evanlucas@me.com>
|
Landed as 07ca288. |
nodejs#11225 introduce an unnecessary bind() when closing a stream. This PR replaces that bind() with a top-level function. PR-URL: nodejs#13474 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Evan Lucas <evanlucas@me.com>
#11225 introduce an unnecessary bind() when closing a stream. This PR replaces that bind() with a top-level function. PR-URL: #13474 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Evan Lucas <evanlucas@me.com>
|
This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land |
#11225 introduce an unnecessary
bind() when closing a stream. This PR replaces that bind() with a
top-level function.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
fs
edit: updated according to @refack suggestion.