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: add final method #12828

Closed
wants to merge 1 commit into from

Conversation

calvinmetcalf
Copy link
Contributor

@calvinmetcalf calvinmetcalf commented May 4, 2017

Adds the ability to for write streams to have an _final method which acts
similarly to the _flush method that transform streams have but is called before
the finish event is emitted and if asynchronous delays the stream from
finishing. The final option may also be passed in order to set it.

this is a new version of #2314 as discussed at nodejs/readable-stream#275, cc @nodejs/streams

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)

@calvinmetcalf calvinmetcalf added the stream Issues and PRs related to the stream subsystem. label May 4, 2017
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label May 4, 2017
@vsemozhetbyt
Copy link
Contributor

if (typeof stream._final === 'function') {
state.pendingcb++;
state.finalCalled = true;
process.nextTick(() =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please use braces here for multi-line arrow function bodies?

state.prefinished = true;
stream.emit('prefinish');
finishMaybe(stream, state);
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing semicolon.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is still the function argument, it seems no semicolon is needed here (linter is OK).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, CI is still down so I cannot see the test results.

@mscdex
Copy link
Contributor

mscdex commented May 4, 2017

New CI post-Jenkins restart: https://ci.nodejs.org/job/node-test-pull-request/7865/


When the stream ends this function will be called before the stream closes,
useful if you need to close a resource or write some data that you had buffered.
This function is completely optional to implement.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should mention in the _flush documentation how the two relate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please avoid using you in the documentation :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should mention in the _flush documentation how the two relate

Mentioning this with flush asks the question, why would one use flush instead of this, and I'd probably recommend using this instead of flush but we had decided to postpone the discussion on whether to soft depreciate flush or not.


const t = new stream.Transform({
objectMode: true,
transform: function(chunk, _, next) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think practically all of the callbacks here should be wrapped in common.mustCall(function[, n])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing as how this specific callback could be called multiple times, I'm not sure we should use one in this case (so as not to rely on streams' internal behavior as much).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In object mode it should definitely be called a fixed number of times, I think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, for object mode perhaps it will be.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: I've been stewing over a modification for mustCall that would allow us to say 'at least once' as opposed to a fixed count

Copy link
Contributor

@mscdex mscdex May 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell I'd probably prefer a separate method to make it more easy to tell vs. a flag or extra argument for mustCall().

assert.strictEqual(++state, chunk + 2, 'transformCallback part 2');
process.nextTick(next);
},
final: function(done) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for switching between done and next? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

traditionally I use next for the callback with transform and done for the name with flush so final seemed like it would be similar to flush.

@addaleax
Copy link
Member

addaleax commented May 4, 2017

@calvinmetcalf
Copy link
Contributor Author

pushed more commits that remove the second person pronoun from the docs, fix a doc link, and fixes some style issues, I will be happy squash these l before we merge

@calvinmetcalf
Copy link
Contributor Author

@addaleax are these CITGM errors related to the pull?

@addaleax
Copy link
Member

addaleax commented May 9, 2017

@calvinmetcalf No, that look “fine” :)

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one suggestion that should definitely be part of this PR. I would prefer for the common.mustCall() wraps to be added here too, but if not, somebody else can do that afterwards.

@@ -1359,6 +1364,19 @@ The `writable._writev()` method is prefixed with an underscore because it is
internal to the class that defines it, and should never be called directly by
user programs.

#### writable.\_final(callback)
Copy link
Member

@addaleax addaleax May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed, ignore me please add ``` ```

directly below this heading :)

@calvinmetcalf
Copy link
Contributor Author

common.mustCall()

I can add this if you want, in any palce in particular

@addaleax
Copy link
Member

addaleax commented May 9, 2017

@calvinmetcalf It would make sense to use that for any callbacks that are invoked a fixed number of times (and at least those callbacks containing assertions)

@calvinmetcalf
Copy link
Contributor Author

ok I'll update the tests

@addaleax
Copy link
Member

addaleax commented May 9, 2017

if (typeof stream._final === 'function') {
state.pendingcb++;
state.finalCalled = true;
process.nextTick(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure we should call this with nextTick?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, let me see if I can come up with a test that effects it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah so if that is call synchronously, then if the callback in final is called synchronously then the finish event will be called before the _flush method

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, would you mind extracting those closures into top-level functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not at all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you also talking about stream._final((err) => { one because I'm not sure we can extract that one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could put a handler on writable state I guess

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep stream._final((err) => { as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that would involve a bind so I'm not sure if that's a win overall

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@calvinmetcalf
Copy link
Contributor Author

ok I'll squash

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a couple of suggestions

* `callback` {Function} Call this function (optionally with an error
argument) when you are done writing any remaining data.

Note: **This function MUST NOT be called directly.** It MAY be implemented
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Note*: This function **must not** be called directly...


When the stream ends this function will be called before the stream closes,
useful if to close a resource or write some data that that is buffered.
This function is completely optional to implement.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omit completely

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also that that is buffered.

Suggested reword:

This optional function will be called before the stream closes, delaying the finish event until callback is called. This is useful to close resources or write buffered data before a stream ends.

@mcollina
Copy link
Member

mcollina commented May 9, 2017

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

(this also need a run with CITGM with and without the READABLE_STREAM flag)

@calvinmetcalf
Copy link
Contributor Author

@jasnell pushed up your doc fixes

@calvinmetcalf
Copy link
Contributor Author

@TomFrost updated with your suggestion

by child classes, and if so, will be called by the internal Writable
class methods only.

This optional function will be called before the stream closes, delaying the `finish` event until `callback` is called. This is useful to close resources or write buffered data before a stream ends.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please line break at <= 80 chars

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. This LGTM. If CI is green and @nodejs/streams signs off it should be good to go as far as I'm concerned.

@lpinca
Copy link
Member

lpinca commented May 22, 2017

It seems unrelated. That test basically writes to a destroyed socket and expects the socket.write() callback to have an error.

@refack
Copy link
Contributor

refack commented May 22, 2017

I had to kill https://ci.nodejs.org/job/citgm-smoker/814/nodes=fedora22/ it got stuck (maybe at ws). IMHO 815 would be enough. killed 815 too. There was nothing of note in the previous run on fedora22, and it's a post EOL platform anyway.

@refack
Copy link
Contributor

refack commented May 22, 2017

CITGM clean 👍

@calvinmetcalf
Copy link
Contributor Author

ok are we waiting on anything for this one ?

@mcollina
Copy link
Member

You can go ahead and land.

A fresh CI: https://ci.nodejs.org/job/node-test-pull-request/8281/

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve CITGM is clean.
Rubber stamp code.

@calvinmetcalf
Copy link
Contributor Author

ba513d1

calvinmetcalf added a commit that referenced this pull request May 24, 2017
Adds the ability to for write streams to have an _final method which acts
similarly to the _flush method that transform streams have but is called before
the finish event is emitted and if asynchronous delays the stream from
finishing.  The `final` option may also be passed in order to set it.

PR-URL: #12828
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@marshall007
Copy link

@calvinmetcalf FYI, there appear to be duplicate lines on lib/_stream_writable.js#L64-L66.

@mcollina
Copy link
Member

good spot @marshall007!

@calvinmetcalf
Copy link
Contributor Author

woops will open a fix

jasnell pushed a commit that referenced this pull request May 25, 2017
Adds the ability to for write streams to have an _final method which acts
similarly to the _flush method that transform streams have but is called before
the finish event is emitted and if asynchronous delays the stream from
finishing.  The `final` option may also be passed in order to set it.

PR-URL: #12828
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
jasnell pushed a commit that referenced this pull request May 28, 2017
Adds the ability to for write streams to have an _final method which acts
similarly to the _flush method that transform streams have but is called before
the finish event is emitted and if asynchronous delays the stream from
finishing.  The `final` option may also be passed in order to set it.

PR-URL: #12828
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
jasnell added a commit to jasnell/node that referenced this pull request May 29, 2017
* **Async Hooks**
  * The `async_hooks` module has landed in core
    [[`4a7233c178`](nodejs@4a7233c178)]
    [nodejs#12892](nodejs#12892).

* **Buffer**
  * Using the `--pending-deprecation` flag will cause Node.js to emit a
    deprecation warning when using `new Buffer(num)` or `Buffer(num)`.
    [[`d2d32ea5a2`](nodejs@d2d32ea5a2)]
    [nodejs#11968](nodejs#11968).
  * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances
    [[`7eb1b4658e`](nodejs@7eb1b4658e)]
    [nodejs#12141](nodejs#12141).
  * Many `Buffer` methods now accept `Uint8Array` as input
    [[`beca3244e2`](nodejs@beca3244e2)]
    [nodejs#10236](nodejs#10236).

* **Child Process**
  * Argument and kill signal validations have been improved
    [[`97a77288ce`](nodejs@97a77288ce)]
    [nodejs#12348](nodejs#12348),
    [[`d75fdd96aa`](nodejs@d75fdd96aa)]
    [nodejs#10423](nodejs#10423).
  * Child Process methods accept `Uint8Array` as input
    [[`627ecee9ed`](nodejs@627ecee9ed)]
    [nodejs#10653](nodejs#10653).

* **Console**
  * Error events emitted when using `console` methods are now supressed.
    [[`f18e08d820`](nodejs@f18e08d820)]
    [nodejs#9744](nodejs#9744).

* **Dependencies**
  * The npm client has been updated to 5.0.0
    [[`3c3b36af0f`](nodejs@3c3b36af0f)]
    [nodejs#12936](nodejs#12936).
  * V8 has been updated to 5.8 with forward ABI stability to 6.0
    [[`60d1aac8d2`](nodejs@60d1aac8d2)]
    [nodejs#12784](nodejs#12784).

* **Domains**
  * Native `Promise` instances are now `Domain` aware
    [[`84dabe8373`](nodejs@84dabe8373)]
    [nodejs#12489](nodejs#12489).

* **Errors**
  * We have started assigning static error codes to errors generated by Node.js.
    This has been done through multiple commits and is still a work in
    progress.

* **File System**
  * The utility class `fs.SyncWriteStream` has been deprecated
    [[`7a55e34ef4`](nodejs@7a55e34ef4)]
    [nodejs#10467](nodejs#10467).
  * The deprecated `fs.read()` string interface has been removed
    [[`3c2a9361ff`](nodejs@3c2a9361ff)]
    [nodejs#9683](nodejs#9683).

* **HTTP**
  * Improved support for userland implemented Agents
    [[`90403dd1d0`](nodejs@90403dd1d0)]
    [nodejs#11567](nodejs#11567).
  * Outgoing Cookie headers are concatenated into a single string
    [[`d3480776c7`](nodejs@d3480776c7)]
    [nodejs#11259](nodejs#11259).
  * The `httpResponse.writeHeader()` method has been deprecated
    [[`fb71ba4921`](nodejs@fb71ba4921)]
    [nodejs#11355](nodejs#11355).
  * New methods for accessing HTTP headers have been added to `OutgoingMessage`
    [[`3e6f1032a4`](nodejs@3e6f1032a4)]
    [nodejs#10805](nodejs#10805).

* **Lib**
  * All deprecation messages have been assigned static identifiers
    [[`5de3cf099c`](nodejs@5de3cf099c)]
    [nodejs#10116](nodejs#10116).
  * The legacy `linkedlist` module has been removed
    [[`84a23391f6`](nodejs@84a23391f6)]
    [nodejs#12113](nodejs#12113).

* **N-API**
  * Experimental support for the new N-API API has been added
    [[`56e881d0b0`](nodejs@56e881d0b0)]
    [nodejs#11975](nodejs#11975).

* **Process**
  * Process warning output can be redirected to a file using the
    `--redirect-warnings` command-line argument
    [[`03e89b3ff2`](nodejs@03e89b3ff2)]
    [nodejs#10116](nodejs#10116).
  * Process warnings may now include additional detail
    [[`dd20e68b0f`](nodejs@dd20e68b0f)]
    [nodejs#12725](nodejs#12725).

* **REPL**
  * REPL magic mode has been deprecated
    [[`3f27f02da0`](nodejs@3f27f02da0)]
    [nodejs#11599](nodejs#11599).

* **Src**
  * `NODE_MODULE_VERSION` has been updated to 57
    (nodejs@ec7cbaf266)]
    [nodejs#12995](nodejs#12995).
  * Add `--pending-deprecation` command-line argument and
    `NODE_PENDING_DEPRECATION` environment variable
    [[`a16b570f8c`](nodejs@a16b570f8c)]
    [nodejs#11968](nodejs#11968).
  * The `--debug` command-line argument has been deprecated. Note that
    using `--debug` will enable the *new* Inspector-based debug protocol
    as the legacy Debugger protocol previously used by Node.js has been
    removed. [[`010f864426`](nodejs@010f864426)]
    [nodejs#12949](nodejs#12949).
  * Throw when the `-c` and `-e` command-line arguments are used at the same
    time [[`a5f91ab230`](nodejs@a5f91ab230)]
    [nodejs#11689](nodejs#11689).
  * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line
    arguments are used at the same time.
    [[`8a7db9d4b5`](nodejs@8a7db9d4b5)]
    [nodejs#12087](nodejs#12087).

* **Stream**
  * `Stream` now supports `destroy()` and `_destroy()` APIs
    [[`b6e1d22fa6`](nodejs@b6e1d22fa6)]
    [nodejs#12925](nodejs#12925).
  * `Stream` now supports the `_final()` API
    [[`07c7f198db`](nodejs@07c7f198db)]
    [nodejs#12828](nodejs#12828).

* **TLS**
  * The `rejectUnauthorized` option now defaults to `true`
    [[`348cc80a3c`](nodejs@348cc80a3c)]
    [nodejs#5923](nodejs#5923).
  * The `tls.createSecurePair()` API now emits a runtime deprecation
    [[`a2ae08999b`](nodejs@a2ae08999b)]
    [nodejs#11349](nodejs#11349).
  * A runtime deprecation will now be emitted when `dhparam` is less than
    2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)]
    [nodejs#11447](nodejs#11447).

* **URL**
  * The WHATWG URL implementation is now a fully-supported Node.js API
    [[`d080ead0f9`](nodejs@d080ead0f9)]
    [nodejs#12710](nodejs#12710).

* **Util**
  * `Symbol` keys are now displayed by default when using `util.inspect()`
    [[`5bfd13b81e`](nodejs@5bfd13b81e)]
    [nodejs#9726](nodejs#9726).
  * `toJSON` errors will be thrown when formatting `%j`
    [[`455e6f1dd8`](nodejs@455e6f1dd8)]
    [nodejs#11708](nodejs#11708).
  * Convert `inspect.styles` and `inspect.colors` to prototype-less objects
    [[`aab0d202f8`](nodejs@aab0d202f8)]
    [nodejs#11624](nodejs#11624).
  * The new `util.promisify()` API has been added
    [[`99da8e8e02`](nodejs@99da8e8e02)]
    [nodejs#12442](nodejs#12442).

* **Zlib**
  * Support `Uint8Array` in Zlib convenience methods
    [[`91383e47fd`](nodejs@91383e47fd)]
    [nodejs#12001](nodejs#12001).
  * Zlib errors now use `RangeError` and `TypeError` consistently
    [[`b514bd231e`](nodejs@b514bd231e)]
    [nodejs#11391](nodejs#11391).
jasnell added a commit that referenced this pull request May 30, 2017
* **Async Hooks**
  * The `async_hooks` module has landed in core
    [[`4a7233c178`](4a7233c178)]
    [#12892](#12892).

* **Buffer**
  * Using the `--pending-deprecation` flag will cause Node.js to emit a
    deprecation warning when using `new Buffer(num)` or `Buffer(num)`.
    [[`d2d32ea5a2`](d2d32ea5a2)]
    [#11968](#11968).
  * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances
    [[`7eb1b4658e`](7eb1b4658e)]
    [#12141](#12141).
  * Many `Buffer` methods now accept `Uint8Array` as input
    [[`beca3244e2`](beca3244e2)]
    [#10236](#10236).

* **Child Process**
  * Argument and kill signal validations have been improved
    [[`97a77288ce`](97a77288ce)]
    [#12348](#12348),
    [[`d75fdd96aa`](d75fdd96aa)]
    [#10423](#10423).
  * Child Process methods accept `Uint8Array` as input
    [[`627ecee9ed`](627ecee9ed)]
    [#10653](#10653).

* **Console**
  * Error events emitted when using `console` methods are now supressed.
    [[`f18e08d820`](f18e08d820)]
    [#9744](#9744).

* **Dependencies**
  * The npm client has been updated to 5.0.0
    [[`3c3b36af0f`](3c3b36af0f)]
    [#12936](#12936).
  * V8 has been updated to 5.8 with forward ABI stability to 6.0
    [[`60d1aac8d2`](60d1aac8d2)]
    [#12784](#12784).

* **Domains**
  * Native `Promise` instances are now `Domain` aware
    [[`84dabe8373`](84dabe8373)]
    [#12489](#12489).

* **Errors**
  * We have started assigning static error codes to errors generated by Node.js.
    This has been done through multiple commits and is still a work in
    progress.

* **File System**
  * The utility class `fs.SyncWriteStream` has been deprecated
    [[`7a55e34ef4`](7a55e34ef4)]
    [#10467](#10467).
  * The deprecated `fs.read()` string interface has been removed
    [[`3c2a9361ff`](3c2a9361ff)]
    [#9683](#9683).

* **HTTP**
  * Improved support for userland implemented Agents
    [[`90403dd1d0`](90403dd1d0)]
    [#11567](#11567).
  * Outgoing Cookie headers are concatenated into a single string
    [[`d3480776c7`](d3480776c7)]
    [#11259](#11259).
  * The `httpResponse.writeHeader()` method has been deprecated
    [[`fb71ba4921`](fb71ba4921)]
    [#11355](#11355).
  * New methods for accessing HTTP headers have been added to `OutgoingMessage`
    [[`3e6f1032a4`](3e6f1032a4)]
    [#10805](#10805).

* **Lib**
  * All deprecation messages have been assigned static identifiers
    [[`5de3cf099c`](5de3cf099c)]
    [#10116](#10116).
  * The legacy `linkedlist` module has been removed
    [[`84a23391f6`](84a23391f6)]
    [#12113](#12113).

* **N-API**
  * Experimental support for the new N-API API has been added
    [[`56e881d0b0`](56e881d0b0)]
    [#11975](#11975).

* **Process**
  * Process warning output can be redirected to a file using the
    `--redirect-warnings` command-line argument
    [[`03e89b3ff2`](03e89b3ff2)]
    [#10116](#10116).
  * Process warnings may now include additional detail
    [[`dd20e68b0f`](dd20e68b0f)]
    [#12725](#12725).

* **REPL**
  * REPL magic mode has been deprecated
    [[`3f27f02da0`](3f27f02da0)]
    [#11599](#11599).

* **Src**
  * `NODE_MODULE_VERSION` has been updated to 57
    (ec7cbaf266)]
    [#12995](#12995).
  * Add `--pending-deprecation` command-line argument and
    `NODE_PENDING_DEPRECATION` environment variable
    [[`a16b570f8c`](a16b570f8c)]
    [#11968](#11968).
  * The `--debug` command-line argument has been deprecated. Note that
    using `--debug` will enable the *new* Inspector-based debug protocol
    as the legacy Debugger protocol previously used by Node.js has been
    removed. [[`010f864426`](010f864426)]
    [#12949](#12949).
  * Throw when the `-c` and `-e` command-line arguments are used at the same
    time [[`a5f91ab230`](a5f91ab230)]
    [#11689](#11689).
  * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line
    arguments are used at the same time.
    [[`8a7db9d4b5`](8a7db9d4b5)]
    [#12087](#12087).

* **Stream**
  * `Stream` now supports `destroy()` and `_destroy()` APIs
    [[`b6e1d22fa6`](b6e1d22fa6)]
    [#12925](#12925).
  * `Stream` now supports the `_final()` API
    [[`07c7f198db`](07c7f198db)]
    [#12828](#12828).

* **TLS**
  * The `rejectUnauthorized` option now defaults to `true`
    [[`348cc80a3c`](348cc80a3c)]
    [#5923](#5923).
  * The `tls.createSecurePair()` API now emits a runtime deprecation
    [[`a2ae08999b`](a2ae08999b)]
    [#11349](#11349).
  * A runtime deprecation will now be emitted when `dhparam` is less than
    2048 bits [[`d523eb9c40`](d523eb9c40)]
    [#11447](#11447).

* **URL**
  * The WHATWG URL implementation is now a fully-supported Node.js API
    [[`d080ead0f9`](d080ead0f9)]
    [#12710](#12710).

* **Util**
  * `Symbol` keys are now displayed by default when using `util.inspect()`
    [[`5bfd13b81e`](5bfd13b81e)]
    [#9726](#9726).
  * `toJSON` errors will be thrown when formatting `%j`
    [[`455e6f1dd8`](455e6f1dd8)]
    [#11708](#11708).
  * Convert `inspect.styles` and `inspect.colors` to prototype-less objects
    [[`aab0d202f8`](aab0d202f8)]
    [#11624](#11624).
  * The new `util.promisify()` API has been added
    [[`99da8e8e02`](99da8e8e02)]
    [#12442](#12442).

* **Zlib**
  * Support `Uint8Array` in Zlib convenience methods
    [[`91383e47fd`](91383e47fd)]
    [#12001](#12001).
  * Zlib errors now use `RangeError` and `TypeError` consistently
    [[`b514bd231e`](b514bd231e)]
    [#11391](#11391).
jasnell added a commit that referenced this pull request May 30, 2017
* **Async Hooks**
  * The `async_hooks` module has landed in core
    [[`4a7233c178`](4a7233c178)]
    [#12892](#12892).

* **Buffer**
  * Using the `--pending-deprecation` flag will cause Node.js to emit a
    deprecation warning when using `new Buffer(num)` or `Buffer(num)`.
    [[`d2d32ea5a2`](d2d32ea5a2)]
    [#11968](#11968).
  * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances
    [[`7eb1b4658e`](7eb1b4658e)]
    [#12141](#12141).
  * Many `Buffer` methods now accept `Uint8Array` as input
    [[`beca3244e2`](beca3244e2)]
    [#10236](#10236).

* **Child Process**
  * Argument and kill signal validations have been improved
    [[`97a77288ce`](97a77288ce)]
    [#12348](#12348),
    [[`d75fdd96aa`](d75fdd96aa)]
    [#10423](#10423).
  * Child Process methods accept `Uint8Array` as input
    [[`627ecee9ed`](627ecee9ed)]
    [#10653](#10653).

* **Console**
  * Error events emitted when using `console` methods are now supressed.
    [[`f18e08d820`](f18e08d820)]
    [#9744](#9744).

* **Dependencies**
  * The npm client has been updated to 5.0.0
    [[`3c3b36af0f`](3c3b36af0f)]
    [#12936](#12936).
  * V8 has been updated to 5.8 with forward ABI stability to 6.0
    [[`60d1aac8d2`](60d1aac8d2)]
    [#12784](#12784).

* **Domains**
  * Native `Promise` instances are now `Domain` aware
    [[`84dabe8373`](84dabe8373)]
    [#12489](#12489).

* **Errors**
  * We have started assigning static error codes to errors generated by Node.js.
    This has been done through multiple commits and is still a work in
    progress.

* **File System**
  * The utility class `fs.SyncWriteStream` has been deprecated
    [[`7a55e34ef4`](7a55e34ef4)]
    [#10467](#10467).
  * The deprecated `fs.read()` string interface has been removed
    [[`3c2a9361ff`](3c2a9361ff)]
    [#9683](#9683).

* **HTTP**
  * Improved support for userland implemented Agents
    [[`90403dd1d0`](90403dd1d0)]
    [#11567](#11567).
  * Outgoing Cookie headers are concatenated into a single string
    [[`d3480776c7`](d3480776c7)]
    [#11259](#11259).
  * The `httpResponse.writeHeader()` method has been deprecated
    [[`fb71ba4921`](fb71ba4921)]
    [#11355](#11355).
  * New methods for accessing HTTP headers have been added to `OutgoingMessage`
    [[`3e6f1032a4`](3e6f1032a4)]
    [#10805](#10805).

* **Lib**
  * All deprecation messages have been assigned static identifiers
    [[`5de3cf099c`](5de3cf099c)]
    [#10116](#10116).
  * The legacy `linkedlist` module has been removed
    [[`84a23391f6`](84a23391f6)]
    [#12113](#12113).

* **N-API**
  * Experimental support for the new N-API API has been added
    [[`56e881d0b0`](56e881d0b0)]
    [#11975](#11975).

* **Process**
  * Process warning output can be redirected to a file using the
    `--redirect-warnings` command-line argument
    [[`03e89b3ff2`](03e89b3ff2)]
    [#10116](#10116).
  * Process warnings may now include additional detail
    [[`dd20e68b0f`](dd20e68b0f)]
    [#12725](#12725).

* **REPL**
  * REPL magic mode has been deprecated
    [[`3f27f02da0`](3f27f02da0)]
    [#11599](#11599).

* **Src**
  * `NODE_MODULE_VERSION` has been updated to 57
    (ec7cbaf266)]
    [#12995](#12995).
  * Add `--pending-deprecation` command-line argument and
    `NODE_PENDING_DEPRECATION` environment variable
    [[`a16b570f8c`](a16b570f8c)]
    [#11968](#11968).
  * The `--debug` command-line argument has been deprecated. Note that
    using `--debug` will enable the *new* Inspector-based debug protocol
    as the legacy Debugger protocol previously used by Node.js has been
    removed. [[`010f864426`](010f864426)]
    [#12949](#12949).
  * Throw when the `-c` and `-e` command-line arguments are used at the same
    time [[`a5f91ab230`](a5f91ab230)]
    [#11689](#11689).
  * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line
    arguments are used at the same time.
    [[`8a7db9d4b5`](8a7db9d4b5)]
    [#12087](#12087).

* **Stream**
  * `Stream` now supports `destroy()` and `_destroy()` APIs
    [[`b6e1d22fa6`](b6e1d22fa6)]
    [#12925](#12925).
  * `Stream` now supports the `_final()` API
    [[`07c7f198db`](07c7f198db)]
    [#12828](#12828).

* **TLS**
  * The `rejectUnauthorized` option now defaults to `true`
    [[`348cc80a3c`](348cc80a3c)]
    [#5923](#5923).
  * The `tls.createSecurePair()` API now emits a runtime deprecation
    [[`a2ae08999b`](a2ae08999b)]
    [#11349](#11349).
  * A runtime deprecation will now be emitted when `dhparam` is less than
    2048 bits [[`d523eb9c40`](d523eb9c40)]
    [#11447](#11447).

* **URL**
  * The WHATWG URL implementation is now a fully-supported Node.js API
    [[`d080ead0f9`](d080ead0f9)]
    [#12710](#12710).

* **Util**
  * `Symbol` keys are now displayed by default when using `util.inspect()`
    [[`5bfd13b81e`](5bfd13b81e)]
    [#9726](#9726).
  * `toJSON` errors will be thrown when formatting `%j`
    [[`455e6f1dd8`](455e6f1dd8)]
    [#11708](#11708).
  * Convert `inspect.styles` and `inspect.colors` to prototype-less objects
    [[`aab0d202f8`](aab0d202f8)]
    [#11624](#11624).
  * The new `util.promisify()` API has been added
    [[`99da8e8e02`](99da8e8e02)]
    [#12442](#12442).

* **Zlib**
  * Support `Uint8Array` in Zlib convenience methods
    [[`91383e47fd`](91383e47fd)]
    [#12001](#12001).
  * Zlib errors now use `RangeError` and `TypeError` consistently
    [[`b514bd231e`](b514bd231e)]
    [#11391](#11391).
this.finalCalled = false;

// if _final has been called
this.finalCalled = false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you write it twice? Hmm.

Copy link
Member

@mcollina mcollina Jun 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very likely a mistake. This has already been fixed.
https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L61-L63

if (typeof stream._final === 'function') {
state.pendingcb++;
state.finalCalled = true;
process.nextTick(callFinal, stream, state);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Is process.nextTick really needed here? Can someone confirm/deny?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szmarczak This looks like a classic case where process.nextTick() is called for-- it's a case where the final function may be sync or async. This post explains more about it. https://nodejs.org/en/docs/guides/event-loop-timers-and-nexttick/#process-nexttick

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what process.nextTick does :P Well, in my case, _final() is sync and process.nextTick() is useless. I haven't checked if that's required by some other Node code, so I'm just asking :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I found some other cases that require moving it to next tick. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet