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

media upload with read stream buffers the entire file into RAM #236

Closed
agoode opened this issue Jul 30, 2014 · 3 comments · Fixed by #331
Closed

media upload with read stream buffers the entire file into RAM #236

agoode opened this issue Jul 30, 2014 · 3 comments · Fixed by #331
Assignees
Labels
🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@agoode
Copy link

agoode commented Jul 30, 2014

I am doing this:

drive.files.insert({
    resource: {
    title: 'test.mp4'
    },
    media: {
    mimeType: 'application/octet-stream',
    body: stream
    }
}, view);

test.mp4 is 3.7GB. top shows that node is using 3.862GB of RAM. I noticed this when I used progress-stream and saw it go immediately to 100%.

@rakyll
Copy link
Contributor

rakyll commented Jul 30, 2014

It seems like sandwich-stream is ignoring the max number of bytes it should read at this line https://github.com/connrs/node-sandwich-stream/blob/master/lib/sandwich-stream.js#L28, ends up buffering the stream until the end.

@ryanseys, we should be able to pipe fs.ReadableStream to req with minimal effort (https://github.com/GoogleCloudPlatform/gcloud-node/blob/master/lib/storage/index.js#L259). We can drop the dependencies.

@ryanseys
Copy link
Contributor

I do pipe the readable stream if there's no multipart required (no resource). If we need to form the multipart body, I use multipart-stream to build the streamable multipart and pipe that to req.

@rakyll
Copy link
Contributor

rakyll commented Jul 30, 2014

I use multipart-stream to build the streamable multipart and pipe that to req.

Yes, but multipart-stream buffers the entire file into memory because SandwichStream is a broken readable stream implementation.

Basic media uploads should be working fine when #235 is merged, and I'll try to fix multipart uploads when I have more bandwidth.

@ryanseys ryanseys added the bug label Jul 31, 2014
@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 This issue needs some love. triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants