Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Converting the Sharp/Node Duplex to web standard ReadableStream #4234

Open
FunctionDJ opened this issue Oct 12, 2024 · 5 comments
Open

Converting the Sharp/Node Duplex to web standard ReadableStream #4234

FunctionDJ opened this issue Oct 12, 2024 · 5 comments
Labels

Comments

@FunctionDJ
Copy link

FunctionDJ commented Oct 12, 2024

Question about an existing feature

What are you trying to achieve?

I'm trying to convert a Sharp object as returned by sharp("...").webp() to a web standard ReadableStream<Uint8Array> for compatibility with Deno.serve() / new Response(body)

When you searched for similar issues, what did you find that might be related?

I have searched a lot, e.g. in the "issues" tab on this repo, but couldn't find any truly related issue. (Aside from #4013 maybe)
I managed to make it work with:

  • new ReadableStream: new ReadableStream({ start(controller) { sharpObject.on("data", chunk => controller.enqueue(chunk) } }) - but it's slightly slower than the classic sharpObject.pipe(res) you'd use with http.createServer or express
  • Readable.toWeb(sharpObject) - appears to pipe some image data, but the served image appears to be broken/blank and it's significantly slower (~3x) on a 20MB input file than .pipe(res) or the previous new ReadableStream option
    (Note: toWeb was added in Node v17 and is still marked as experimental)
  • createReadableStreamFromReadable from @remix-run/node works but is ~3x as slow as .pipe(res). They also use their own new ReadableStream. This function can be used with import { createReadableStreamFromReadable } from "@remix-run/node" even when not using the Remix framework.

Somewhat related: Stackoverflow posts about the difference between web standard ReadableStream and Node Readable

Please provide a minimal, standalone code sample, without other dependencies, that demonstrates this question

const sharpObject = sharp("img.png").webp();
const readableStream: ReadableStream<Uint8Array> = sharpObject;
/**
 * TypeScript will already warn here that
 * ` 'Sharp' is missing the following properties from type 'ReadableStream<Uint8Array>' `,
 * which at runtime holds true as the `readableStream` variable doesn't work
 * with e.g. `new Response(readableStream)` in Deno
 */

Please provide sample image(s) that help explain this question

independent of image file

@lovell
Copy link
Owner

lovell commented Oct 12, 2024

Readable.toWeb(sharpObject) - appears to pipe some image data, but the served image appears to be broken/blank

Use of toWeb is probably what I'd suggest people try first when converting. Are you able to provide a minimal code sample that allows someone else to reproduce the problem you ran into?

@FunctionDJ
Copy link
Author

FunctionDJ commented Oct 14, 2024

@lovell
I'm using a large, 20MB PNG file for these tests, with only 1 request at a time. I haven't tested multiple/mass requests yet and it would make the reproduction more complicated but mass requests is my actual use case which is why i'm considering performance.

This first example works both when run with Deno and with Node and has equal performance between the two JS runtimes on average in my test case:

import http from "node:http";
import sharp from "sharp";

http
	.createServer((_req, res) => {
		const sharpObject = sharp("img.png").webp();
		sharpObject.pipe(res);
	})
	.listen(8000);

This second example only works in Deno and is about 1% slower on average. More importantly, the response appears to be incomplete. Chromium will log GET http://localhost:8000/ net::ERR_INCOMPLETE_CHUNKED_ENCODING 200 (OK) in the console and the bottom ~15% of the image appear as gray and Postman reports ~800KB response size vs. the 1MB response of the first example.

import sharp from "npm:sharp";
import { Readable } from "node:stream";

Deno.serve(() => {
	const sharpObject = sharp("img.png").webp();
	return new Response(Readable.toWeb(sharpObject) as ReadableStream);
});

With the original post and the 3x performance difference i've mentioned, i wasn't able to reproduce this today so i must have made a mistake in input files when i did the previous testing. Please excuse me.

@lovell
Copy link
Owner

lovell commented Oct 14, 2024

This looks a bit like the problem reported at denoland/deno#24606

If performance is of concern and memory is not a limiting factor, perhaps experiment with Buffers rather than Streams to avoid many small memory allocations.

@FunctionDJ
Copy link
Author

FunctionDJ commented Oct 14, 2024

I found that Deno issue too, but i'm not using the Fresh framework or http2 (request protocol is http/1.1 according to Chromium Devtools).

As for Buffers, i'm not aware that they can be streamed. If i use sharpObject.toBuffer(), then the code will wait for the conversion to finish, keeping all data in memory for the moment (if i understood correctly).

I've also talked to contributors on the Deno side and expectedly they told me to ask on the package (sharp) side to look into this issue.

@ducan-ne
Copy link

ducan-ne commented Oct 24, 2024

Bun is working with the following code

import sharp from "sharp"
import { Readable } from "node:stream"
import { Buffer } from "node:buffer"

Bun.serve({
  fetch: async (req) => {
    const svg = await req.text()
    const sharpObject = sharp(Buffer.from(svg)).webp()
    return new Response(Readable.toWeb(sharpObject) as ReadableStream, {
      headers: {
        'Content-Type': 'image/webp',
      },
    })
  },
})

But it getting this error message for each request, not sure why it occurs

error: Premature close
 code: "ERR_STREAM_PREMATURE_CLOSE"

      at new NodeError (node:stream:244:20)
      at node:stream:714:47
      at emit (node:events:48:48)
      at emit (node:events:48:48)
      at endReadableNT (node:stream:2207:27)
      ```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants
@lovell @ducan-ne @FunctionDJ and others