Promise version of streams.finished not calling clean up

[rhodgkins]

node/lib/internal/streams/end-of-stream.js

Lines 245 to 255 in f0cf100

	function finished(stream, opts) {
	return new Promise((resolve, reject) => {
	eos(stream, opts, (err) => {
	if (err) {
	reject(err);
	} else {
	resolve();
	}
	});
	});
	}

Should the promise version of finished call the clean up function returned from the callback version?

So something like:

function finished(stream, opts) {
  return new Promise((resolve, reject) => {
    const cleanup = eos(stream, opts, (err) => {
      cleanup();
      if (err) {
        reject(err);
      } else {
        resolve();
      }
    });
  });
}

[mcollina]

cc @nodejs/streams

[mcollina]

@ronag wdyt?

[mcollina]

Would you like to send a PR?

[rhodgkins]

Yeh can do, I've never contributed to this repo before - would tests or anything be needed or will just the above change be ok?

[mcollina]

Test will be needed.

[ronag]

Note that changing this semver-major. I would prefer if we added an option to the promisified version, .e.g autoCleanup: true or something like that which we can later change the default of if/when we want as semver-major change.

[tony-go]

Hey 👋

If our friend @rhodgkins finally doesn't jump on this. I would like to jump :)

Cheers

[mcollina]

Go for it!

[rhodgkins]

Yep sorry I'm not going to be able to do this any time soon so thanks in advance if you get time to do it! 🎉

[tony-go]

It seems that @ntedgi already jump into. I'll take another one 🤓

[targos]

Fixed by #44862