uNetworking / uWebSockets.js

μWebSockets for Node.js back-ends :metal:
Apache License 2.0
8.08k stars 573 forks source link

[Bug]: uWebSockets.js file upload high ram usage #529

Closed dalisoft closed 3 years ago

dalisoft commented 3 years ago

Hi @alexhultman

I tried uWebSockets.js file upload with getParts(buffer, contentType) and memory usage is same as uploading file or more. How i can reduce memory usage? Can be situation where server has 2GiB limit and uploading file is 4GiB

dalisoft commented 3 years ago

Snippet was copied from https://github.com/uNetworking/uWebSockets.js/discussions/106#discussioncomment-401457

Napzu commented 3 years ago

Just use fs.createWriteStream.

uWSapp.post('/upload', async (res, req) => {
    console.log('STREAMING CLIENT UPLOAD TO FILE');
    res.writeHeader('Access-Control-Allow-Origin', req.getHeader('origin')).onAborted(e => console.error('UPLOAD ABORTED') );    
    let writeStream = fs.createWriteStream('myfile.extension');
    await new Promise((resolve,reject) => {
        writeStream.on('error', reject)
        res.onData( (ab, isLast) => {
            writeStream.write(Buffer.from(ab));
            isLast && resolve();
        })
    });
    console.log('STREAMING DONE!');
    res.end();
})
dalisoft commented 3 years ago

@Napzu Thanks, but your snippet does not work. I maintain own library on top of uWebSockets.js and know it's possible doing with other ways (which not one, e.g. busboy). But author of uWebSockets.js knows about optimizations and performance better than anyone in the world, so i'd like to learn to how to implement this in right way from author of this library

Napzu commented 3 years ago

I tested that code before posting.

Worked fine with uws 18.14.0

dalisoft commented 3 years ago

@Napzu Please look at https://github.com/uNetworking/uWebSockets.js/discussions/251 for why it does not work

Napzu commented 3 years ago

Strange, on non SSL server there where same issues as with your #251 issue.

But accounting that referencing issue streaming works just fine.

Here is easier code to test that out quickly:

var uWS = require('uWebSockets.js');
let fs = require('fs'), streamBuffer = false;
uWS.App().listen(8080,_=>console.log('Listening streaming magic on port 8080'))
.get('/',(res, req)=>{
    res.end(`
        <html><head></head><body>
            <input id="file" type="file"><br><br>
            <button onclick="upload()">UPLOAD JPG</button><br><br>
            <a id="a" style="display:none;" href="/myPic.jpg" download>Download uploaded file</a>
            <script>
                let a = document.getElementById('a')
                function upload() {
                    a.insertAdjacentHTML('beforebegin','UPLOADING IMAGE!<br><br>')
                    fetch('', {
                        method : 'POST',
                        headers: { "Content-Type": "multipart/form-data;" },
                        body: document.getElementById('file').files[0]
                    }).then(()=>{
                        a.insertAdjacentHTML('beforebegin','<b>UPLOADING DONE!!!</b><br><br>')
                        a.style.display = ''
                    })
                }
            </script>
        </body></html>`)
}).get('/myPic.jpg', res => res.end(streamBuffer))
.post('/*', async (res, req) => {
    console.log('STREAMING CLIENT UPLOAD TO FILE');
    res.writeHeader('Access-Control-Allow-Origin', req.getHeader('origin')).onAborted(e => console.error('UPLOAD ABORTED') );
    streamBuffer = Buffer.alloc(0)
    await new Promise((resolve,reject) => {
        res.onData( (ab, isLast) => {
            streamBuffer = Buffer.concat([streamBuffer,Buffer.from(ab)])
            isLast && resolve()
        })
    });
    console.log('STREAMING DONE!');
    res.end();
})
krm35 commented 3 years ago

@Napzu your method works with small file but if the file size exceeds the memory, it won't because you store it into the memory.

// const uWS = require("../dist/uws.js");
const uWS = require('uWebSockets.js');
const querystring = require("querystring");
const fs = require("fs");
const port = 9001;

const app = uWS
    ./*SSL*/ App({
        key_file_name: "misc/key.pem",
        cert_file_name: "misc/cert.pem",
        passphrase: "1234"
    })
    .get('/', (res, req) => {
        res.end(`
        <html lang="en"><body>
            <input id="file" type="file"><br><br>
            <button onclick="upload()">UPLOAD</button><br><br>
            <script>
                function upload() {
                    fetch('upload?fileName=fileName.yourExtension', {
                        method : 'POST',
                        headers: { "Content-Type": "multipart/form-data;" },
                        body: document.getElementById('file').files[0]
                    })
                }
            </script>
        </body></html>`)
    })
    .post('/upload', async (res, req) => {
        const {fileName} = {...querystring.parse(req.getQuery())};
        res.writeHeader('Access-Control-Allow-Origin', req.getHeader('origin'))
            .onAborted(() => console.error('UPLOAD ABORTED'));
        let writeStream = fs.createWriteStream(fileName);
        await new Promise((resolve, reject) => {
            writeStream.on('error', reject);
            res.onData((chunk, isLast) => {
                writeStream.write(Buffer.from(chunk.slice(0)));
                isLast && resolve();
            })
        });
        res.end('file uploaded');
    })
    .listen(port, token => {
        if (token) {
            console.log("Listening to port " + port);
        } else {
            console.log("Failed to listen to port " + port);
        }
    });
Napzu commented 3 years ago

Sure thing. It was just proof of concept to stream to variable. Just replace variable with writestream( which you did) and you are good to go.

krm35 commented 3 years ago

@dalisoft btw you can't use getParts() with big files because it will only work with files smaller than the memory of the server cauz you need to concatanate all chunks until the last one to call the method.

        const header = req.getHeader('content-type');
        await new Promise(resolve => {
            let buffer = Buffer.from('');
            res.onData((ab, isLast) => {
                const chunk = Buffer.from(ab);
                buffer = Buffer.concat([buffer, chunk]);
                if (isLast) {
                    const data = getParts(buffer, header);
                    console.log(data);
                    // do something with data
                    resolve();
                }
            })
        });
dalisoft commented 3 years ago

Thank you @krm35

Already understood that’s and the reason why i now use Formidable. Here my code part which used getParts and seems it’s not efficient like Formidable

ghost commented 3 years ago

If getParts takes buffer then obviously you have to...... have the buffer. So yes, it will indeed take up memory for the buffer since you have to have it. This is for simplicity; streaming parser should be used if you want to upload huge files as multipart

dalisoft commented 3 years ago

Thank you, then i using right code snippet