New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: avoid emitting error EBADF for double close #11225

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@mcollina
Member

mcollina commented Feb 7, 2017

Changed the logic in fs.ReadStream and fs.WriteStream so that
close always calls the prototype method rather than the internal
event listener.

Fixes: #2950

cc @evanlucas @Trott @bnoordhuis

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs, stream

@nodejs-github-bot nodejs-github-bot added the fs label Feb 7, 2017

@jasnell

jasnell approved these changes Feb 7, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 7, 2017

Member

Given that this affects error handling it technically should be considered for semver-major. However, I think it qualifies as a bug fix so leaving it as semver-patch.

Member

jasnell commented Feb 7, 2017

Given that this affects error handling it technically should be considered for semver-major. However, I think it qualifies as a bug fix so leaving it as semver-patch.

Show outdated Hide outdated test/parallel/test-fs-write-stream-double-close.js
fs: avoid emitting error EBADF for double closeo
Changed the logic in fs.ReadStream and fs.WriteStream so that
close always calls the prototype method rather than the internal
event listener.

Fixes: #2950
@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Feb 7, 2017

Member

@jasnell I added the don't land on LTS labels, just in case.

Member

mcollina commented Feb 7, 2017

@jasnell I added the don't land on LTS labels, just in case.

@cjihrig

cjihrig approved these changes Feb 7, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 7, 2017

Contributor

Minor nit: there's a typo in the commit message

Contributor

mscdex commented Feb 7, 2017

Minor nit: there's a typo in the commit message

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 7, 2017

Contributor

Another reason not to land on v4.x and v6.x is the use of .bind(), which won't be very fast in those versions.

Contributor

mscdex commented Feb 7, 2017

Another reason not to land on v4.x and v6.x is the use of .bind(), which won't be very fast in those versions.

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Feb 7, 2017

Member

If we aren't planning on backporting this, I think we should go ahead and mark as semver-major (especially given that it seems like a breaking change)

Member

evanlucas commented Feb 7, 2017

If we aren't planning on backporting this, I think we should go ahead and mark as semver-major (especially given that it seems like a breaking change)

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Feb 8, 2017

Member

Another reason not to land on v4.x and v6.x is the use of .bind(), which won't be very fast in those versions.

We can just use an arrow function instead?

Member

Fishrock123 commented Feb 8, 2017

Another reason not to land on v4.x and v6.x is the use of .bind(), which won't be very fast in those versions.

We can just use an arrow function instead?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Feb 8, 2017

Member

Another reason not to land on v4.x and v6.x is the use of .bind(), which won't be very fast in those versions.

We can just use an arrow function instead?

If that's the only problem, yes of course.

Member

mcollina commented Feb 8, 2017

Another reason not to land on v4.x and v6.x is the use of .bind(), which won't be very fast in those versions.

We can just use an arrow function instead?

If that's the only problem, yes of course.

@mcollina mcollina changed the title from fs: avoid emitting error EBADF for double closeo to fs: avoid emitting error EBADF for double close Feb 13, 2017

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Feb 13, 2017

Member

Landed as b1fc774

Member

mcollina commented Feb 13, 2017

Landed as b1fc774

@mcollina mcollina closed this Feb 13, 2017

mcollina added a commit that referenced this pull request Feb 13, 2017

fs: avoid emitting error EBADF for double close
Changed the logic in fs.ReadStream and fs.WriteStream so that
close always calls the prototype method rather than the internal
event listener.

Fixes: #2950
PR-URL: #11225
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

fs: avoid emitting error EBADF for double close
Changed the logic in fs.ReadStream and fs.WriteStream so that
close always calls the prototype method rather than the internal
event listener.

Fixes: nodejs#2950
PR-URL: nodejs#11225
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Jun 1, 2017

Member

@jasnell @mcollina Was this intentionally left as semver-major?

Member

Fishrock123 commented Jun 1, 2017

@jasnell @mcollina Was this intentionally left as semver-major?

@mcollina mcollina deleted the mcollina:fix-2950 branch Jun 2, 2017

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Jun 2, 2017

Member

@Fishrock123 yes. I'm happy for it to be backported, but we were not in agreement. If we agree that this is not semver-major, we can backport. I can fire a PR.

cc @jasnell @evanlucas @mscdex @cjihrig

Member

mcollina commented Jun 2, 2017

@Fishrock123 yes. I'm happy for it to be backported, but we were not in agreement. If we agree that this is not semver-major, we can backport. I can fire a PR.

cc @jasnell @evanlucas @mscdex @cjihrig

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Jun 3, 2017

Contributor

I don't know if this is pressing enough that it needs to be backported, but I don't feel strongly either way.

Contributor

cjihrig commented Jun 3, 2017

I don't know if this is pressing enough that it needs to be backported, but I don't feel strongly either way.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jun 3, 2017

Contributor

@mcollina Now that I look at this again, I think we could have avoided the bind() with just:

this.once('open', this.close);

which is more easily backportable. Maybe submit a separate PR for the bind() removal and then backport this and that PR at the same time?

Contributor

mscdex commented Jun 3, 2017

@mcollina Now that I look at this again, I think we could have avoided the bind() with just:

this.once('open', this.close);

which is more easily backportable. Maybe submit a separate PR for the bind() removal and then backport this and that PR at the same time?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Jun 5, 2017

Member

@mscdex we cannot, because open will be emitted with the fd as the first argument. If we want to change this, we need to change the check on cb, so that it ignores anything that is not a function. As calling close()  when a stream is not open yet is likely not a hot path, I don't think it's worth the added complexity.

Member

mcollina commented Jun 5, 2017

@mscdex we cannot, because open will be emitted with the fd as the first argument. If we want to change this, we need to change the check on cb, so that it ignores anything that is not a function. As calling close()  when a stream is not open yet is likely not a hot path, I don't think it's worth the added complexity.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jun 5, 2017

Contributor

@mcollina Ok well we could still pull out a reusable function and do it that way:

this.once('open', closeOnOpen);

// ...

function closeOnOpen() {
  this.close(null);
}
Contributor

mscdex commented Jun 5, 2017

@mcollina Ok well we could still pull out a reusable function and do it that way:

this.once('open', closeOnOpen);

// ...

function closeOnOpen() {
  this.close(null);
}

@mcollina mcollina referenced this pull request Jun 5, 2017

Closed

fs: replace a bind() with a top-level function #13474

2 of 2 tasks complete
@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Jun 5, 2017

Member

here it is: #13474

Member

mcollina commented Jun 5, 2017

here it is: #13474

mcollina added a commit to mcollina/node that referenced this pull request Jun 7, 2017

fs: replace a bind() with a top-level function
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>

mcollina added a commit to mcollina/node that referenced this pull request Jun 7, 2017

fs: replace a bind() with a top-level function
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>

jasnell added a commit that referenced this pull request Jun 7, 2017

fs: replace a bind() with a top-level function
#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment