Output stream is not properly ended after .clone()

[mvolfik]

Are you using the latest version? Is the version currently in use as reported by npm ls sharp the same as the latest version as reported by npm view sharp dist-tags.latest?

Yes, using sharp v0.29.1

What are the steps to reproduce?

This is partially related to #2897. When a sharp 'instance' is .clone()d and you attempt to consume the clone with .pipe() (native Node stream API), the stream isn't properly finalized (no matter what data input method is used for the original constructor, that's why I believe it's a separate issue.

What is the expected behaviour?

When you clone a sharp instance and specify it to drain into a pipe, it should call this.push(null) at the end to mark the stream as finished.

Are you able to provide a minimal, standalone code sample, without other dependencies, that demonstrates this problem?

import sharp from "sharp";
import fs from "node:fs";
import { createHash } from "node:crypto";
import { promisify } from "node:util";
import { finished } from "node:stream";

function sleep(x) {
  return new Promise((resolve) => {
    setTimeout(resolve, x);
  });
}

async function hashStream(s, workaround) {
  const h = createHash("sha1");
  s.pipe(h);
  await (workaround ? sleep(5000) : promisify(finished)(s));
  return h.digest("hex");
}

(async () => {
  const s = fs.createReadStream("content/main/assets/hero.jpg").pipe(sharp());
  hashStream(s.clone()).then((x) => console.log("After clone, no workaround", x));
  hashStream(s.clone(), true).then((x) => console.log("After clone, with sleep workaround", x));
  hashStream(s).then((x) => console.log("Original, no workaround", x));
})();

In the output of this code, only the latter two promises resolve and get logged (both with the same hash).

Are you able to provide a sample image that helps explain the problem?

Not relevant

What is the output of running npx envinfo --binaries --system?

  System:
    OS: Linux 5.4 Ubuntu 20.04.3 LTS (Focal Fossa)
    CPU: (4) x64 Intel(R) Core(TM) i3-3110M CPU @ 2.40GHz
    Memory: 291.26 MB / 7.35 GB
    Container: Yes
    Shell: 3.1.0 - /usr/bin/fish
  Binaries:
    Node: 14.17.6 - /usr/bin/node
    Yarn: 1.22.11 - ~/.local/bin/yarn
    npm: 7.21.0 - ~/.local/bin/npm

[lovell]

Does making the following change, which ensures clone is called before pipe, help?

-  const s = fs.createReadStream("content/main/assets/hero.jpg").pipe(sharp());
+  const s = sharp();
   hashStream(s.clone()).then((x) => console.log("After clone, no workaround", x));
   hashStream(s.clone(), true).then((x) => console.log("After clone, with sleep workaround", x));
   hashStream(s).then((x) => console.log("Original, no workaround", x));
+  fs.createReadStream("content/main/assets/hero.jpg").pipe(s);

If so, perhaps we need to guard against the use of clone after pipe?

[mvolfik]

No, that doesn't seem to change anything, which I think should be correct - the pipe API should be async-like (do not block with major immediate data transfer) I think, that's why there are the finished callbacks etc

[lovell]

Thanks for checking. It looks like sample code works as expected with Node.js 12, but not from 14 onwards, so I guess the default behaviour of Duplex Streams altered. There are a few semver major changes related to Streams listed in https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V14.md#semver-major-commits

It looks like fixing this (and #2897) should be as simple as ensuring this.emit('close') is called straight after both uses of this.push(null). PR very welcome, if you're able.

[lovell]

v0.30.0 now available with this, thanks Drian for the PR.