Skip to content

stream: expect _destroy to call the callback in thennables

Rodrigo Muino Tomonari requested to merge github/fork/marmitar/issue/40377 into master

Update _destroy to only call onDestroy in case of errors, waiting for the user implementation to call onDestroy on the success path of async functions as well. This is already what happens with Readable._read.

Fixes: #40377 (closed)

Description of the problem

As @szmarczak pointed out, currenlty on master, if we have have some synchronous code like:

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

class SyncDestroy extends Writable {
    _destroy(error, callback) {
        console.log('Calling the callback soon...')
        setTimeout(() => callback(error), 0)
    }
}

const writer = new SyncDestroy()
writer.on('error', error => console.log(`${error}`))
writer.destroy(new Error('oh no!'))

Then we have the expected output of:

Calling the callback soon...
Error: oh no!

However, changing _destroy to async changes the behaviour unexpectedly.

class AsyncDestroy extends Writable {
    async _destroy(error, callback) {
        console.log('Calling the callback soon...')
        setTimeout(() => callback(error), 0)
    }
}

const writer = new AsyncDestroy()
writer.on('error', error => console.log(`${error}`))
writer.destroy(new Error('oh no!'))
Calling the callback soon...

Merge request reports