Skip to content
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

stream: emit finish when using writev and cork #13195

Closed
wants to merge 1 commit into from

Conversation

@mcollina
Copy link
Member

commented May 24, 2017

In Writable, 'finish' was not emitted when using writev() and
cork() in the event of an Error. This commit makes it consistent with the write() path,
which emits 'finish'.

Fixes: #11121

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

stream

@mcollina mcollina requested review from mscdex and calvinmetcalf May 24, 2017

@mcollina

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

@mcollina

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

cc @cynron

@Fishrock123
Copy link
Member

left a comment

@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 24, 2017

There's a typo in the commit message. Also it might be worthwhile to note in the test and commit message that it's only missing when an error occurs/is passed.

@mcollina mcollina force-pushed the mcollina:finish-writev branch May 24, 2017

@mcollina

This comment has been minimized.

Copy link
Member Author

commented May 24, 2017

@mscdex updated.

@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 24, 2017

It looks like there were linting errors during the last CI run.

Here's CI again though: https://ci.nodejs.org/job/node-test-pull-request/8283/

@cynron

This comment has been minimized.

Copy link
Contributor

commented May 25, 2017

👌

@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 25, 2017

@mcollina Looks like there's still linter errors

stream: emit finish when using writev and cork
In Writable, 'finish' was not emitted when using writev() and
cork() in the event of an Error during the write. This commit
makes it consistent with the write() path, which emits 'finish'.

Fixes: #11121

@mcollina mcollina force-pushed the mcollina:finish-writev branch to c335929 May 25, 2017

@mcollina

This comment has been minimized.

Copy link
Member Author

commented May 25, 2017

@mscdex weird, https://ci.nodejs.org/job/node-test-linter/9327/console is not showing up what the problem is.

I've amended a line ending, something went wrongly on my box.

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

@refack

This comment has been minimized.

Copy link
Member

commented May 25, 2017

@mscdex weird, https://ci.nodejs.org/job/node-test-linter/9327/console is not showing up what the problem is.

Ref: nodejs/build#720 (I have it in my TODO list to fix tools/jslint.js)

@mcollina

This comment has been minimized.

Copy link
Member Author

commented May 26, 2017

Landed as b153420.

@mcollina mcollina closed this May 26, 2017

@mcollina mcollina deleted the mcollina:finish-writev branch May 26, 2017

mcollina added a commit that referenced this pull request May 26, 2017
stream: emit finish when using writev and cork
In Writable, 'finish' was not emitted when using writev() and
cork() in the event of an Error during the write. This commit
makes it consistent with the write() path, which emits 'finish'.

Fixes: #11121
PR-URL: #13195
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell added a commit that referenced this pull request May 28, 2017
stream: emit finish when using writev and cork
In Writable, 'finish' was not emitted when using writev() and
cork() in the event of an Error during the write. This commit
makes it consistent with the write() path, which emits 'finish'.

Fixes: #11121
PR-URL: #13195
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
mcollina added a commit to mcollina/node that referenced this pull request Jun 21, 2017
stream: finish must always follow error
When _write completes with an Error, 'finish' was emitted before
'error' if the callback was asynchronous. This commit restore the
previous behavior.
The logic is still less then ideal, because we call the write()
callback before emitting error if asynchronous, but after if
synchronous. This commit do not try to change the behavior.
This commit fixes a regression introduced by:
nodejs#13195.

Fixes: nodejs#13812
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 21, 2017

@nodejs/LTS

We should not land this in v6.x yet

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2017

@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 21, 2017

This should land with #13850 (assuming that lands), but as Myles says should probably wait another release or two.

mcollina added a commit that referenced this pull request Jun 22, 2017
stream: finish must always follow error
When _write completes with an Error, 'finish' was emitted before
'error' if the callback was asynchronous. This commit restore the
previous behavior.
The logic is still less then ideal, because we call the write()
callback before emitting error if asynchronous, but after if
synchronous. This commit do not try to change the behavior.
This commit fixes a regression introduced by:
#13195.

Fixes: #13812
PR-URL: #13850
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Jun 24, 2017
stream: finish must always follow error
When _write completes with an Error, 'finish' was emitted before
'error' if the callback was asynchronous. This commit restore the
previous behavior.
The logic is still less then ideal, because we call the write()
callback before emitting error if asynchronous, but after if
synchronous. This commit do not try to change the behavior.
This commit fixes a regression introduced by:
#13195.

Fixes: #13812
PR-URL: #13850
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mcollina mcollina referenced this pull request Jul 18, 2017
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.