Readable._construct behaviour contradicts docs

[mikuso]

Version

v16.8.0

Platform

Microsoft Windows NT 10.0.19043.0 x64

Subsystem

stream

What steps will reproduce the bug?

Returning a promise from Readable._construct has the same effect as calling the callback. This is not documented and may be a bug?

This leads to the following strange behaviour:

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

const stream = new Readable({
    async construct(callback) {
        callback();
    },
    read(size) {
        console.log('read', size);
    },
    destroy(err, callback) {
        console.log('destroy', err);
    }
});

Output:

destroy Error [ERR_MULTIPLE_CALLBACK]: Callback called multiple times
    at new NodeError (node:internal/errors:371:5)
    at onConstruct (node:internal/streams/destroy:255:37)
    at processTicksAndRejections (node:internal/process/task_queues:82:21) {
  code: 'ERR_MULTIPLE_CALLBACK'
}

Similarly:

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

const stream = new Readable({
    async construct(callback) {
        // don't call the callback
    },
    read(size) {
        console.log('read');
    }
});

stream.read();

Output:

read

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

Flow should not proceed to Readable._read until after the callback passed to Readble._construct has been called.

The docs state:

This optional function will be scheduled in the next tick by the stream constructor, delaying any _read() and _destroy() calls until callback is called.

What do you see instead?

• In the first example, an error is thrown for calling the callback twice (despite only calling it once)
• In the second example, flow passes to Readable._read despite the callback not being called yet.

Additional information

It is desirable to be able to be able to use await within Readable._construct, but labelling this function as async (and therefore returning a Promise) appears to cause the callback function to be implicitly called (which is sometimes unwanted).

As a workaround, I can return my own Promise from Readable._construct and use its resolve/reject functions in lieu of the callback.

In summary, either this is expected behaviour and the docs are incorrect - or vice versa. It's hard to tell.

[lpinca]

It is not made explicit but you either return a promise or call the callback, not both.

[ronag]

Maybe docs could be improved to be more explicit about this.

[zamnuts]

This "feature" was introduced by #34416 (technically via 744a284). It includes both _construct, _transform, _flush, _final, and _destroy. (Lots of duplication in the impl, bump #34723.) So this is generally applicable for Readable, Writable, and Duplex/Transform interfaces.

They all check if the method is a thenable, then loosely callbackifies it. For example, for _construct, this is by means of the internal destroy implementation:

node/lib/internal/streams/destroy.js

Lines 288 to 305 in 253f934

	const result = stream._construct(onConstruct);
	if (result != null) {
	const then = result.then;
	if (typeof then === 'function') {
	then.call(
	result,
	function() {
	if (!called) {
	process.nextTick(onConstruct, null);
	}
	},
	function(err) {
	if (!called) {
	process.nextTick(onConstruct, err);
	}
	});
	}
	}

Unless I'm reading this wrong, I wonder how the Duplex tests are passing (are they?) since they use both async _construct and callback without hitting ERR_MULTIPLE_CALLBACK, e.g.:

node/test/parallel/test-stream-construct-async-error.js

Lines 66 to 70 in afe460e

	async _construct(callback) {
	// eslint-disable-next-line no-restricted-syntax
	await setTimeout(common.platformTimeout(1));
	callback();
	}

Aside, I also noticed that @types/node@16 shows _construct():void for both Readable and Writable:

• Readable: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/48773b6236aeb1ced8e799e414e33968a7199b03/types/node/stream.d.ts#L117
• Writable: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/48773b6236aeb1ced8e799e414e33968a7199b03/types/node/stream.d.ts#L553

I'm having trouble figuring out of this is a supported feature / public API, or us stream implementers just got lucky. It is not documented anywhere, yet all the issues/commits/PRs I'm finding discuss it like it is an existing/known feature and there is a test suite available.

Maybe docs could be improved to be more explicit about this.

Do you mean it is a signature with a stability index of 2 ("stable")?