Stream is not properly ended on file -> stream

[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, sharp v0.29.1

What are the steps to reproduce?

You can create a sharp instance from filename/buffer directly, or you can 'pipe' it into a sharp instance. The same goes for output - you can call sharp.toBuffer(), or you can use the stream API to pipe the result somewhere (though it still gets outputted in 1 large buffer/chunk, so there isn't any benefit to it. However, when you're using file input, but stream output, the stream isn't properly EOF'd - readable.push(null) probably never happens, even though all data were already sent.

Here's the output of running the code below - the Finished debug log doesn't appear in the file stream case

$ yarn node test.js stream stream
yarn node v1.22.11
0.152982641 Create
0.16096844 Rotate
0.161259813 Pipe
0.161377642 Output request
0.162478042 End
2.01617784 Chunk of size 3476488
2.019047609 Finished
Done in 2.12s.

$ yarn node test.js file stream
yarn node v1.22.11
0.163400974 Create
0.170362431 Rotate
0.170752708 Pipe
0.171065131 Output request
0.172855515 End
2.249066901 Chunk of size 3476488
Done in 2.36s.

$ yarn node test.js stream file
yarn node v1.22.11
0.149069651 Create
0.157169212 Rotate
0.157422221 Pipe
0.157552227 Output request
0.158138876 End
2.19398687 Finished
2.947392881 Chunk of size 3476488
Done in 3.23s.

$ yarn node test.js file file
yarn node v1.22.11
0.151799809 Create
0.159130976 Rotate
0.15936061 Pipe
0.159631797 Output request
0.160491234 End
2.488794328 Finished
2.505566336 Chunk of size 3476488
Done in 2.62s.

What is the expected behaviour?

The output stream pipe is properly finished even if the input is a file.

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 { finished } from "node:stream";

const fn = "a.jpg";

console.log(process.uptime(), "Create");

let s;
if (process.argv[2] === "stream") {
  const fh = fs.createReadStream(fn);
  s = fh.pipe(sharp());
} else {
  s = sharp(fn);
}

console.log(process.uptime(), "Rotate");

s.rotate(90);

console.log(process.uptime(), "Pipe");

s.on("data", (x) => console.log(process.uptime(), "Chunk of size", x.length));

console.log(process.uptime(), "Output request");

if (process.argv[3] === "stream") {
  s.pipe(fs.createWriteStream("/dev/null"));
  finished(s, () => {
    console.log(process.uptime(), "Finished");
  });
} else {
  s.toBuffer().then(() => {
    console.log(process.uptime(), "Finished");
  });
}

console.log(process.uptime(), "End");

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

Not applicable (happens with multiple images i tried).

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: 754.32 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]

Hi, thanks for the detailed report. If you hadn't seen it, the call to .push(null) occurs here for the combination of filesystem input and Stream output:

sharp/lib/output.js

Lines 1060 to 1069 in 2679bb5

	// output=stream, input=file/buffer
	sharp.pipeline(this.options, (err, data, info) => {
	if (err) {
	this.emit('error', err);
	} else {
	this.emit('info', info);
	this.push(data);
	}
	this.push(null);
	});

Making the following change to your test code seems to produce the expected output:

 if (process.argv[3] === "stream") {
-  s.pipe(fs.createWriteStream("/dev/null"));
-  finished(s, () => {
+  const w = s.pipe(fs.createWriteStream("/dev/null"));
+  finished(w, () => {
     console.log(process.uptime(), "Finished");
   });

...which I think makes sense as the WritableStream should be responsible for emitting the finish event rather than the ReadableStream, or at least this behaviour matches the Node.js docs - see https://nodejs.org/api/stream.html#stream_class_stream_readable

[mvolfik]

Hi, thanks for the responses. I saw that piece of code as well and tried some basic debugging, but didn't find any easy fix (or what even is the root issue).

Changing the code like you suggest does work, however, I'd argue this is not the correct behavior - Node docs say it should work for readable streams as well, and even provide examples of using finalize on readable streams...

I guess you can make the call if this issue should stay open as a bug.
I'll look into your response to the other issue I reported in a minute

[lovell]

Making the following change, to explicitly emit the close event, appears to satisfy the test code, although I'm unsure why this is not the default behaviour as I believe ReadableStream instances should always emit close when they are destroyed.

           this.push(data);
         }
         this.push(null);
+        this.emit('close');
       });
     }
     return this;

Happy to accept a PR with this change and an associated test that would previously have failed, if you're able.

[lovell]

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