v16.5.0 change in async writable._final() behaviour

[tchetwin]

Version

16.5.0

Platform

Microsoft Windows NT 10.0.19041.0 x64

Subsystem

stream

What steps will reproduce the bug?

As a result of #39329, a program that works in v16.4.2 no longer works in v16.5.0.
The issue occurs when an async writable._final() implementation explicitly calls the callback argument.
In v16.5.0 the callback-call and async-resolve combined result in [ERR_MULTIPLE_CALLBACK]: Callback called multiple times in some circumstances.

import { Duplex, Readable } from "stream";
import { setTimeout } from "timers/promises";

const slowDuplex = new Duplex({
    read(_size) {
        setTimeout(1000).then(() => {
            this.push("abc\n");
            this.push(null);
        })
    },
    async final(cb) {
        // both this `cb()` and the `_final()` Promise resolving
        // will trigger `onFinish` under the hood.
        cb();
    },
});

slowDuplex.on("error", (err) => {
    console.log(`"error" fired\n  ${err}`);
});

Readable.from([]).pipe(slowDuplex).pipe(process.stdout);

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

Always

What is the expected behavior?

$ nvm run 16.4.2 repro.mjs
Running node v16.4.2 (npm v7.18.1)
abc

What do you see instead?

$ nvm run 16.5.0 repro.mjs
Running node v16.5.0 (npm v7.19.1)
"error" fired
  Error [ERR_MULTIPLE_CALLBACK]: Callback called multiple times

Additional information

Documentation

I don't believe the _final() documentation sets expectations about the behaviour when it is async (or a Promise returner). The source code before and after the change in question seems to point towards it being an intended means of implementing the _final().

(Unless I've missed a part of the documentation): Users equipped only with this documentation and without looking at the implementation may be relying on the previous behaviour.

Types

The DefinitelyTyped types could potentially encode this expectation with an overload for void-returners having the argument, and Promise-returners not having the argument.

[targos]

@nodejs/streams

[ronag]

IMO the current behavior is correct. I think both async and cb should error like this but maybe not on v16 and docs might need some improvement.

Maybe a v16 fix or revert?

[mcollina]

The quickest solution is do not use async in streams method. Unless noted, the API was designed in a pre-promise era and it might not like the fact that async returns a Promise.

(We should probably put this in the doc).

[ronag]

and it might not like the fact that async returns a Promise.

Ouch, yea, we are missing a Promise.resolve(thenable) to ensure recursive resolve. Adding promise support was maybe not the best idea :/.

[robpalme]

For clarity, the above issue is something that affected a home-grown Gulp-based compiler used in production. We've already fixed the compiler itself, so it is now compatible with Node 16.5, so all is well for new releases.

Unfortunately this behavior-change will block upgrading the machine that runs the compilation service to use Node >16.4 until we've patched all packages that use old versions of the compiler. Those packages are not owned by us, so it means convincing other teams to do work. I'd estimate it would delay our upgrade by around one year. This is inconvenient but not fatal - I pass it on not to request any particular action/work, purely for awareness. Ultimately, it looks like the behavior-change is the right way forwards, so it should go ahead either now or later.

[mcollina]

@ronag I think we should do a revert :( and possibly land the change in v17.

@robpalme would that be ok?

[robpalme]

@mcollina A revert would be welcome, but it's not essential. The key is to avoid backporting the break to 14.x.

There's no need to delay progress if you think we're the only party affected here. (I wrote the offending code so it's on me anyway 😉)

[paulrutter]

Any progress on this issue @mcollina?
I'm running into this issue as well, although i haven't been able to isolate it yet in our codebase.

Error [ERR_MULTIPLE_CALLBACK]: Callback called multiple times
    at NodeError (node:internal/errors:371:5)
    at onFinish (node:internal/streams/writable:667:37)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)

Does this go for other stream methods (like transform and flush) as well?
Anything we can do about it, except fixing the code to either use callbacks or promises (e.g. async functions)?

The latter might be hard when using NPM dependencies, so i'd rather have it "fixed" in Node.js where possible.
I still think it should be ok to just use the callback (although not very neat), while making the function itself async, since that makes it easier to use await in those methods.

Node version: 16.13.0

Update
Found the relevant piece of code:

    const responseStream = new Transform({
      objectMode: true,
      transform: (chunk, enc, next) => next(null, chunk),
      final: cb =>
        myInstance.close()
          .then(cb)
          .catch(err => responseStream.emit("error", err))
    });

This causes the final method to return a promise, which in turn will call the onFinish method two times.

[mcollina]

@ronag is on it.

[ronag]

Fixed in afe460e