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: use more explicit statements #13863

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -829,10 +829,11 @@ Node.js does not allow `stdout` or `stderr` Streams to be closed by user code.
Used when an attempt is made to close the `process.stdout` stream. By design,
Node.js does not allow `stdout` or `stderr` Streams to be closed by user code.

<a id="ERR_STREAM_HAS_STRINGDECODER"></a>
### ERR_STREAM_HAS_STRINGDECODER
<a id="ERR_STREAM_WRAP"></a>
### ERR_STREAM_WRAP

Used to prevent an abort if a string decoder was set on the Socket.
Used to prevent an abort if a string decoder was set on the Socket or if in
`objectMode`.

Example
```js
Expand Down
2 changes: 1 addition & 1 deletion lib/_stream_transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ function afterTransform(er, data) {

var cb = ts.writecb;

if (!cb) {
if (cb === null) {
Copy link
Member

Choose a reason for hiding this comment

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

This potentially could be a breaking change given that it would miss other falsy values potentially passed for cb

Copy link
Member

Choose a reason for hiding this comment

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

I checked. We are setting this internally, it's not user-driven.

return this.emit('error', new errors.Error('ERR_MULTIPLE_CALLBACK'));
}

Expand Down
7 changes: 2 additions & 5 deletions lib/_stream_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@ const assert = require('assert');
const util = require('util');
const Socket = require('net').Socket;
const JSStream = process.binding('js_stream').JSStream;
// TODO(bmeurer): Change this back to const once hole checks are
// properly optimized away early in Ignition+TurboFan.
var Buffer = require('buffer').Buffer;
const uv = process.binding('uv');
const debug = util.debuglog('stream_wrap');
const errors = require('internal/errors');
Expand Down Expand Up @@ -47,12 +44,12 @@ function StreamWrap(stream) {
self.emit('error', err);
});
this.stream.on('data', function ondata(chunk) {
if (!(chunk instanceof Buffer)) {
if (typeof chunk === 'string' || this._readableState.objectMode === true) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The object mode check could actually be moved in the StreamWrap constructor but it is currently possible to change the objectMode variable from an existing stream and in that case the upper check would not be sufficient anymore.
It is explicitly noted in the docs that this is not safe but I'm thinking about opening a PR to make the objectMode read only.

@mcollina do you think that's reasonable?

Copy link
Member

Choose a reason for hiding this comment

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

Streams implementors often change objectMode after the fact, so we cannot assume that. It'd a semver-major change, and several of us are -1 on those.

Copy link
Member

Choose a reason for hiding this comment

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

What you did here is ok.

// Make sure that no further `data` events will happen
this.pause();
this.removeListener('data', ondata);

self.emit('error', new errors.Error('ERR_STREAM_HAS_STRINGDECODER'));
self.emit('error', new errors.Error('ERR_STREAM_WRAP'));
Copy link
Member

Choose a reason for hiding this comment

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

Why this error code change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the error code was very specific and somewhat misleading that objectMode would or should not trigger this even though objectMode always would have resulted in a misleading error since it's first existence.

I think now it better represents both cases. I do see that it's not ideal to rename the code though.

Copy link
Member

Choose a reason for hiding this comment

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

Given that I don't believe the other error code had actually shipped in a release yet, this is fine. Definitely makes this semver-major tho.

Copy link
Member

Choose a reason for hiding this comment

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

The new error code needs to be included in the docs tho

return;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536');
E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed');
E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed');
E('ERR_STREAM_HAS_STRINGDECODER', 'Stream has StringDecoder');
E('ERR_STREAM_WRAP', 'Stream has StringDecoder set or is in objectMode');
E('ERR_TRANSFORM_ALREADY_TRANSFORMING',
'Calling transform done when still transforming');
E('ERR_TRANSFORM_WITH_LENGTH_0',
Expand Down
45 changes: 32 additions & 13 deletions test/parallel/test-stream-wrap-encoding.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,42 @@
'use strict';
const common = require('../common');
const assert = require('assert');

const StreamWrap = require('_stream_wrap');
const Duplex = require('stream').Duplex;

const stream = new Duplex({
read: function() {
},
write: function() {
}
});
{
const stream = new Duplex({
read() {},
write() {}
});

stream.setEncoding('ascii');
stream.setEncoding('ascii');

const wrap = new StreamWrap(stream);
const wrap = new StreamWrap(stream);

wrap.on('error', common.mustCall(function(err) {
assert(/StringDecoder/.test(err.message));
}));
wrap.on('error', common.expectsError({
type: Error,
code: 'ERR_STREAM_WRAP',
message: 'Stream has StringDecoder set or is in objectMode'
}));

stream.push('ohai');
stream.push('ohai');
}

{
const stream = new Duplex({
read() {},
write() {},
objectMode: true
});

const wrap = new StreamWrap(stream);

wrap.on('error', common.expectsError({
type: Error,
code: 'ERR_STREAM_WRAP',
message: 'Stream has StringDecoder set or is in objectMode'
}));

stream.push(new Error('foo'));
}