perf(storage-s3): stream files and abort s3 request from static handler #13430
Conversation
|
Script to validate memory usage: #!/bin/bash
PID=5556
while :; do date +"%T"; ps -o rss=,vsz= -p $PID | awk '{printf "RSS: %.1fMB VSZ: %.1fMB\n",$1/1024,$2/1024}'; sleep 1; doneDownloading a 50MB PDF file twice. Output from script when running a version without the fix: Memory usage goes from 256MB and up to over 400MB when downloading the file twice. Output from script when running with the fix applied: Only going from 274MB and up to 294MB So as you can see this is a big resource and memory saving change. |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes S3 file serving by streaming S3 objects directly to HTTP responses instead of buffering them in memory, while adding abort controller support to properly handle client disconnections.
- Removes the
streamToBufferfunction that was creating full file buffers in memory - Adds abort controller integration to stop S3 downloads when clients abort requests
- Improves type safety for the stream detection helper function
| }) | ||
|
|
||
| streamed = true | ||
| return new Response(stream, { headers, status: 200 }) |
There was a problem hiding this comment.
Returning a Node.js Readable stream directly to Response constructor may not work in all environments. Consider checking if the runtime supports streaming responses or provide a fallback mechanism.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
We should be safe with newer node versions, but not sure about other edge runtimes.
We can wrap it in Readable.toWeb(stream), but not sure about the upside/downside of this.
andershermansen
changed the title
|
Updated with some refactoring to make the code more clear. I have tested the following cases:
For performance it seems it's best to leave the Readable.toWeb out of it if we can. |
|
Would this also solve #12366? |
I don’t know. Unable to reproduce that issue myself, so hard to know. Maybe you can create a canary package with this change and let the people in that issue test? |
|
Fantastic call out @andershermansen! |
|
🚀 This is included in version v3.53.0 |
What?
Stream S3 object directly to response instead of creating a Buffer in memory and wire up an abort controller to stop streaming if user aborts download
Why?
To avoid excessive memory usage and to abort s3 download if user has aborted the request anyway.
How?
In node environment the AWS S3 always returns a Readable. The streamToBuffer method always required this, but the any type hided that this was actually needed. Now there is an explicit type check, but this should never trigger in a node server environment.
Wire up and abort controller to the request so that we tell the S3 object to also stop streaming further if the user aborts.
Fixes #10286
Maybe also helps on other issues with s3 and resource usage