stream: ensure state existed when getting property

[ZYSzys]

Ensure state has initialized when getting property from prototype.
On the other hand, it wouldn't throw TypeError when accessing
readableLength, readableHighWaterMark and e.g. from
stream.Readable.prototype.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3232

[mscdex]

This needs some tests.

[BridgeAR]

This does not really seem necessary to me.

@nodejs/streams PTAL

[BridgeAR]

The call to super should always be done before accessing this.

[ZYSzys]

Honestly speaking, I learned this from destroyed method tests:

node/lib/_stream_readable.js

Lines 182 to 204 in 53e0f63

	Object.defineProperty(Readable.prototype, 'destroyed', {
	// Making it explicit this property is not enumerable
	// because otherwise some prototype manipulation in
	// userland will fail
	enumerable: false,
	get() {
	if (this._readableState === undefined) {
	return false;
	}
	return this._readableState.destroyed;
	},
	set(value) {
	// We ignore the value if the stream
	// has not been initialized yet
	if (!this._readableState) {
	return;
	}
	// Backward compatibility, the user is explicitly
	// managing destroyed
	this._readableState.destroyed = value;
	}
	});

node/test/parallel/test-stream-readable-destroy.js

Lines 155 to 166 in 53e0f63

	{
	function MyReadable() {
	assert.strictEqual(this.destroyed, false);
	this.destroyed = false;
	Readable.call(this);
	}
	Object.setPrototypeOf(MyReadable.prototype, Readable.prototype);
	Object.setPrototypeOf(MyReadable, Readable);
	new MyReadable();
	}

[ZYSzys]

@BridgeAR So should I update the test ? Any advice would be appreciated.

[mcollina]

What is the case where those properties are not initialized? The state is allocated with the stream itself.

[ZYSzys]

The simplest case I'm considering is to get the property from prototype directly instead of throw TypeError.

> stream.Readable.prototype.readableLength
Thrown:
TypeError: Cannot read property 'length' of undefined
    at Stream.get (_stream_readable.js:1082:32)
>

[mcollina]

LGTM

[nodejs-github-bot]

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

[BridgeAR]

I am -0 on this, since I don't think we have to guard against something like this. There should not be a point to access these properties before the class is instantiated. The error itself might not be that helpful but AFAIC it's just something unsupported. If at all, I would probably throw an error that the class has to be instantiated first.

[ZYSzys]

There should not be a point to access these properties before the class is instantiated.

I think someone maybe have a choice to get some information(some properties' default value) from prototype before they instantiate the stream. E.g. getting the default value of highWaterMark from Readable.prototype.readableHighWaterMark before new Readable().

[ZYSzys]

@BridgeAR FYI, it seems that all of our public api's prototype(e.g. events.prototype, net.prototype) properties can be accessed without throwing error, though the default value may be undefined.

So I think maybe this is a point for consistence. Are you ok if I land this ?

[BridgeAR]

@ZYSzys IMO that's more like a implementation detail. Did you actually run into this anywhere in real code?

[ZYSzys]

@BridgeAR Maybe things like:

const { Readable } = require('stream');

// Get the default value of our `readableHighWaterMark`,
// so that we can instantiate our readable stream due to this value.
// For some references ?
const defaultHighWaterMark = Readable.prototype.readableHighWaterMark;

// Twice than the default value
const rs = new Readable({ highWaterMark: defaultHighWaterMark * 2 })

[BridgeAR]

@ZYSzys I don't think a maybe warrants this change. Your example also seems unreliable: most defaults are not set on the prototype but often in the constructor itself.

[ZYSzys]

Your example also seems unreliable: most defaults are not set on the prototype but often in the constructor itself.

Yep 🤔👍...

But at least it wouldn't throw the confused TypeError now...
And maybe we can expose the default value to developers for references in this way.

Wouldn't the default value better than the confused error here at present ？

[BridgeAR]

@ZYSzys did you run into this anywhere in real code? I personally would not bother and I doubt that this is something people really run into.

We could theoretically throw an error that accessing the property is undefined outside of the instance but to me it just adds checks for the default case that are won't help people in real life. At least that's my point of view.

[BridgeAR]

@lpinca @mafintosh @addaleax PTAL. It would be good to have some people who work more frequently on streams giving their opinion in this case.

[lpinca]

I kind of agree with Ruben. I think this actually masks user errors for example when they forget to call the parent constructor. A thrown error is better is this case.
I'm -0.