Writable is destroyed before calling callback in rare case

[szmarczak]

Version

v16.10.0

Platform

Linux solus 5.14.7-198.current #1 SMP PREEMPT Wed Sep 22 16:02:46 UTC 2021 x86_64 GNU/Linux

Subsystem

stream

What steps will reproduce the bug?

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

class X extends Writable {
    async _destroy(error, callback) {
        (async () => {
            await new Promise(resolve => setTimeout(resolve, 10));
            console.log(w._writableState.closed);

            callback(error);
        })();
    }
}

const w = new X();

w.once('error', error => {
    console.log(error);
});

w.destroy(new Error('oh no!'));

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

Always.

What is the expected behavior?

false
Error: oh no!

What do you see instead?

true

Additional information

Some undocumented behavior here:

node/lib/internal/streams/destroy.js

Lines 101 to 118 in aff2a0a

	try {
	const result = self._destroy(err || null, onDestroy);
	if (result != null) {
	const then = result.then;
	if (typeof then === 'function') {
	then.call(
	result,
	function() {
	process.nextTick(onDestroy, null);
	},
	function(err) {
	process.nextTick(onDestroy, err);
	});
	}
	}
	} catch (err) {
	onDestroy(err);
	}

[targos]

@nodejs/streams

[ronag]

Don’t mix thenable with callback.

[mcollina]

The behavior is correct for me. After .destroy() is called, the stream is closed synchronously and no more read or write operation can happen. However the destroy cycle is asynchronous because it might take a while to completely clean up a native resource.

[szmarczak]

@mcollina I forgot to include Error: oh no! in expected behavior. The error is not emitted. The issue is that if _destroy returns a thenable, there's a race condition - either the callback gets called first or the promise resolves. If it waits for the thenable, it should omit the callback argument (or throw when it gets called). Also the documentation doesn't mention anything about _destroy being thenable. Therefore I consider this a bug.

[mcollina]

Uh? Using a thenable there should not be supported.

[ronag]

I think this is a case of insufficient documentation.

[mcollina]

I have a feeling I missed something when we added thenable support to destroy.

I'll need to dig into this and check out what's the problem.

[mcollina]

This was added in 744a284 without documentation.

The error is not printed because it is not rethrown by the _destroy function. Moreover, the callback should not be mixed with async/await (nor is needed).

[ronag]

I think we could maybe emit a warning if function returns a thenable when the function.length has the value for the callback signature.

[szmarczak]

Wouldn't it be better to throw after the promise resolves and the callback gets called?

[mcollina]

Wouldn't it be better to throw after the promise resolves and the callback gets called?

if you implement _destroy(), you need to rethrow or call the callback with the error passed as an argument.

[szmarczak]

Again there's this situation where callback may be called after the promise resolves. Currently calling the callback doesn't do anything because at that point the stream is destroyed already because the promise resolved.

One can expect callback not to throw, so indeed a warning would be sufficient I think.

[mcollina]

The documentation must be updated to better specify all of this. I would recommend to add a warning as well when using a promise with a callback.

[Mesteery]

I think this is a good first issue.

[RafaelGSS]

@mcollina it can be closed #41040 was merged.