stream: Support writing null to stream.Transform #5729

kevinoid · 2016-03-15T22:59:36Z

Here's a proposed fix for #5711, which passes null to ._transform, like other falsey values.

It updates the falsey value test for Transform to include null (and undefined, which was previously omitted) and adds a test for the falsey value behavior of Writable in objectMode, as requested in #5711 (comment)

Let me know what you think @mcollina and @calvinmetcalf (and anyone else interested, of course).

Fixes: #5711

As discussed in nodejs#5711, writing null to a stream.Transform puts the stream into an unrecoverable state. This commit fixes the issue by passing it to ._transform, like the other falsey values. It updates the falsey value test for Transform to include null (and undefined, which was previously omitted) and adds a test for the behavior of Writable in objectMode when falsey values are written to match Transform. Fixes: nodejs#5711

mscdex · 2016-03-15T23:19:18Z

Does this mean that non-objectMode streams can receive non-Buffer/string values now or do they get stringified or what?

kevinoid · 2016-03-15T23:44:56Z

The changes in this PR don't affect non-objectMode streams, since the writes don't make it to ._write().

Currently calling .write(null) causes TypeError: Cannot read property 'length' of null at _stream_writable.js:268. It would probably make more sense to catch it in validChunk, but that change might be better suited for another PR?

kevinoid · 2016-03-15T23:52:44Z

Looking closer, .write(null) for non-objectMode streams increments pendingcb before throwing, so the stream won't ever 'finish'. Looks like a good thing to fix. Should I include it in this PR or roll another for the non-objectMode behavior?

mcollina · 2016-03-16T07:28:56Z

Roll another PR.

We should probably run this on citgm (https://github.com/nodejs/citgm) before landing, just in case.

kevinoid · 2016-03-16T15:41:13Z

@mcollina Sure, can't hurt. Is there a build farm which runs citgm? Is there anything I need to do to schedule a run?

mcollina · 2016-03-16T15:58:10Z

Not sure. Any idea @mafintosh @calvinmetcalf?

jasnell · 2016-03-16T19:45:56Z

@thealphanerd set up a job in ci.nodejs.org that runs citgm. I'm sure he can help get you going with it.

calvinmetcalf · 2016-03-17T18:36:40Z

lib/_stream_transform.js

@@ -162,7 +162,7 @@ Transform.prototype._write = function(chunk, encoding, cb) {
 Transform.prototype._read = function(n) {
  var ts = this._transformState;

-  if (ts.writechunk !== null && ts.writecb && !ts.transforming) {


what was the original rational for this check?

26ca7d7 describes it as "null is reserved to indicate stream eof". Perhaps the author confused .write(null) with .push(null), or planned for .write(null) to cause EOF? Or they intended to disallow .write(null) to avoid confusion about it being interpreted as EOF in other contexts? I'm not exactly sure.

this change also means that writing null to a passthrough will end the readable side which is a bit weird

this change also means that writing null to a passthrough will end the readable side

I don't think so. null and undefined are ignored by the ._transform callback.

ah cool. didn't know that.

MylesBorins · 2016-03-30T00:24:34Z

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/162/

kevinoid · 2016-03-30T00:26:22Z

Thanks @thealphanerd!

mcollina · 2016-03-30T07:13:03Z

Quite a few failures, we should probably have a closer look at them.

mcollina · 2016-03-30T07:41:19Z

Thinking about this, there is a good reason why you cannot pass null to through a Transform stream.
It is the PassThrough scenario.

stream.write(null) -> stream._transform(null, '', cb) -> this.push(null) -> the stream closes

For simmetry, we might even remove writing null to a Writable.

However, we should support the other falsey values (like undefined).

kevinoid · 2016-03-30T19:42:46Z

Indeed. stream.PassThrough works because it passes the written value to the _transform callback, which ignores null and undefined, but it looks like there are plenty of other implementations which pass the written value to push without filtering null.

I can open a new PR which ignores, throws, or emits an error for .write(null). Is there agreement on which of these should occur? I see the following options:

Match the current behavior for non-objectMode streams by throwing TypeError, as mentioned in stream: Support writing null to stream.Transform #5729 (comment) . This seems like unintentional behavior which is likely to cause uncaughtException, but it would be consistent.
Behave like non-string/non-Buffer values written to non-objectMode streams, and emit an error from validChunk. I'm not a big fan of this behavior, since reading code is likely to think the stream has ended with an error while the writing code is likely to keep writing.
Call the write callback without calling _transform as @calvinmetcalf had suggested in stream: Writing null to objectMode Transform is unrecoverable #5711 (comment) . This seems like a bit of a stretch for "successfully writing" the value, but it also seems like the least likely to cause breakage.

My vote is for the last option. In an ideal world, I would like to change validChunk to only emit 'error' when the caller did not provide a callback, to avoid confusing readers listening for 'error', but this would be a backwards-incompatible change that is probably not justified. So my vote is to call the callback without an Error. What do you guys think? Should the behavior of non-objectMode streams be changed too?

mcollina · 2016-03-31T07:21:08Z

@kevinoid I'm not understanding all the various options you are laying out. Can you please clarify?

I think we should emit/callback an Error if null or undefined are passed in write() independently of objectMode. Regarding the fact that the stream might still be used after such an error, this is highly unlikely: an user will need to check the Error' type (or message) and recover from there. It is better to jettison all the stream and restart from scratch.

kevinoid · 2016-03-31T18:17:56Z

@mcollina Sure, that will work. I disagree that the writer continuing is unlikely, since I think there is a lot of code in the wild which doesn't properly check for write errors. But I'm not opposed to that option.

Stated somewhat differently (in the same order), the three options I see are:

Make write(null) in objectMode behave like write(null) in non-objectMode by throwing TypeError.
Make null fail the checks in validChunk, which will emit 'error' and pass Error to callback.
Ignore null writes without emitting or returning Error. Call cb without error on nextTick.

Should I open a PR to implement option 2, as you suggested, or hold off until after it is discussed at the WG meeting?

mcollina · 2016-04-01T08:48:06Z

I'm ok for option 2. Calvin?

Il giorno gio 31 mar 2016 alle 20:19 Kevin Locke [email protected]
ha scritto:

@mcollina https://github.com/mcollina Sure, that will work. I disagree
that the writer continuing is unlikely, since I think there is a lot of
code in the wild which doesn't properly check for write errors. But I'm not
opposed to that option.

Stated somewhat differently (in the same order), the three options I see
are:

Make write(null) in objectMode behave like write(null) in non-
objectMode by throwing TypeError.

Make null fail the checks in validChunk, which will emit 'error'
and pass Error to callback.

Ignore null writes without emitting or returning Error. Call cb
without error on nextTick.

Should I open a PR to implement option 2, as you suggested, or hold off
until after it is discussed at the WG meeting?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5729 (comment)

calvinmetcalf · 2016-04-12T18:26:59Z

at the readable stream meeting yesterday we decide on 2 so I'll open a pull for that

kevinoid · 2016-04-12T20:44:44Z

Sounds good, thanks @calvinmetcalf!

mscdex added the stream Issues and PRs related to the stream subsystem. label Mar 15, 2016

calvinmetcalf reviewed Mar 17, 2016
View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 17, 2016

kevinoid closed this Mar 30, 2016

calvinmetcalf mentioned this pull request Mar 31, 2016

April 2016 working group meeting nodejs/readable-stream#183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: Support writing null to stream.Transform #5729

stream: Support writing null to stream.Transform #5729

kevinoid commented Mar 15, 2016

mscdex commented Mar 15, 2016

kevinoid commented Mar 15, 2016

kevinoid commented Mar 15, 2016

mcollina commented Mar 16, 2016

kevinoid commented Mar 16, 2016

mcollina commented Mar 16, 2016

jasnell commented Mar 16, 2016

calvinmetcalf Mar 17, 2016

kevinoid Mar 17, 2016

mafintosh Mar 17, 2016

kevinoid Mar 17, 2016

mafintosh Mar 17, 2016

MylesBorins commented Mar 30, 2016

kevinoid commented Mar 30, 2016

mcollina commented Mar 30, 2016

mcollina commented Mar 30, 2016

kevinoid commented Mar 30, 2016

mcollina commented Mar 31, 2016

kevinoid commented Mar 31, 2016

mcollina commented Apr 1, 2016

calvinmetcalf commented Apr 12, 2016

kevinoid commented Apr 12, 2016

stream: Support writing null to stream.Transform #5729

stream: Support writing null to stream.Transform #5729

Conversation

kevinoid commented Mar 15, 2016

mscdex commented Mar 15, 2016

kevinoid commented Mar 15, 2016

kevinoid commented Mar 15, 2016

mcollina commented Mar 16, 2016

kevinoid commented Mar 16, 2016

mcollina commented Mar 16, 2016

jasnell commented Mar 16, 2016

calvinmetcalf Mar 17, 2016

Choose a reason for hiding this comment

kevinoid Mar 17, 2016

Choose a reason for hiding this comment

mafintosh Mar 17, 2016

Choose a reason for hiding this comment

kevinoid Mar 17, 2016

Choose a reason for hiding this comment

mafintosh Mar 17, 2016

Choose a reason for hiding this comment

MylesBorins commented Mar 30, 2016

kevinoid commented Mar 30, 2016

mcollina commented Mar 30, 2016

mcollina commented Mar 30, 2016

kevinoid commented Mar 30, 2016

mcollina commented Mar 31, 2016

kevinoid commented Mar 31, 2016

mcollina commented Apr 1, 2016

calvinmetcalf commented Apr 12, 2016

kevinoid commented Apr 12, 2016