Vanilla NodeJS request.on('end') is being called before formidable.parse` completes

[NoelAbrahams]

Support plan

• which support plan is this issue covered by? Community:
• is this issue currently blocking your project? yes
• is this issue affecting a production system? no

Context

• node version: 15.2.1
• module (formidable) version: 1.2.2
• environment (e.g. node, browser, native, OS): node, Windows 10
• used with (i.e. popular names of modules): Vanilla NodeJS
• any other relevant information:

What are you trying to achieve or the steps to reproduce?

Trying to upload a simple file around 100 KB. There is a branch in the code below for handling multi-part form data. We want formidable to handle that. We want to handle all other requests via the second branch.

import * as formidable from 'formidable';

// Handle incoming HTTP requests
httpServer.on('request', (request, response) => {

    let body;

    if (request.headers['content-type'].includes('multipart/form-data')) {

        formidable.parse(request, (error, fields, files) => {
            // Process multi-part form data
            body = files;
        });
    }
    else {
        request.on('data', data => {
            // Process other
            body = data;
        });
    }

    // Handle completion of the read operation above
    request.on('end', async () => {

        // Do something with body
        response.write(body);
    });
    
});

What was the result you got?

This works some of the time. But there is a race condition because the last bit request.on('end') is being called before formidable.parse completes.

What result did you expect?

One that works — I know that's controversial.

[tunnckoCore]

Kind of make sense to me. Everything is event-driven async flow, so... you can't just do that.

Try adding request.on('end') inside the .parse callback:

        formidable.parse(request, (error, fields, files) => {
            // Process multi-part form data
            request.on('end', () => {
              response.write(files);
            });
        });

Also, please try v2 canary (npm install formidable@canary), because v1.2.2 (is actually v1.2.1 with just updated readme) is pretty old and a TON has changed and fixed, almost the whole issue tracker.

And a tip, you can just import formidable from 'formidable'; or import { formidable } from 'formidable';

[NoelAbrahams]

@tunnckoCore thanks for the tips. I've updated to formidable@canary and it's working fine — albeit with the same unexpected behaviour reported above.

I dug into this a bit more and the reason on('end') is being called early is because there is no on('data') call in the multi-part branch. According to this SO answer,

If you attach a data event listener, then it will switch the stream into flowing mode, and data will be passed to your handler as soon as it is available.

The solution I've gone with for the moment is to not listen to the end at all:

// Handle incoming HTTP requests
httpServer.on('request', (request, response) => {

    let body;

    if (request.headers['content-type'].includes('multipart/form-data')) {

        formidable.parse(request, (error, fields, files) => {
            // Process multi-part form data
            body = files;
        });

        // Do something with body
        response.write(body);
    }
    else {
        request.on('data', data => {
            // Process other
            body = data;
        });

       // Handle completion of the read operation above
      request.on('end', async () => {

          // Do something with body
          response.write(body);
     });
   }
  
});

I feel the correct solution is to do something like this:

// Handle incoming HTTP requests
httpServer.on('request', (request, response) => {

    let body;
    const isMultiPart = request.headers['content-type'].includes('multipart/form-data');

    request.on('data', data => {

        if (isMultiPart) {
            body = Buffer.concat([body, data]);
        }
        else {
            body += data;
        }
    });

    // Handle completion of the read operation above
    request.on('end', async () => {

        let result;

        if (isMultiPart) {

            formidable.parse(body, (error, fields, files) => {
                // Process multi-part form data
                result = files;
            });
        }
        else {
            // Parse string body
            result = JSON.parse(body);
        }

        // Do something with the result
        response.write(result);
    });

});

But formidable does not accept a Buffer for processing. I feel this is the correct solution because it reads the data as a stream rather than in one go.

So, some food for thought. I also hope that by not listening to on('end') I'm not doing something terribly wrong.

[tunnckoCore]

I still don't understand why you need to listen to the end, we do that and call the .parse callback, it's almost surely enough to just response.write from the callback.

If you attach a data event listener, then it will switch the stream into flowing mode, and data will be passed to your handler as soon as it is available.

Very true. I remember that and probably it's the reason, yep.

The solution that works for the moment is definitely signaling how it should not be done :D Sorry, don't want to be rude or aggressive or anything, it just doesn't feel and look good, it's not how async flow goes.

---

offtopic...

The whole problem is we are doing a lot more than just the MultipartParser and I kind of hate that, it can be a ton simpler and easier. Haha, anyway.

[NoelAbrahams]

you [don't] need to listen to the end, we do that and call the .parse callback,

Great. That's what I wanted to know.

[Your working solution is not] how it should be done

You've probably not read the code right, because 'not listening to 'end'' is exactly what I too suggested.

The whole problem is we are doing a lot more than just the MultipartParser

Yes, absolutely agree. formidable should ideally not deal with request at all. It should be the caller's responsibility to read the request and then decide what to do with it — either ask formidable to parse it or do something else. This is the solution that I outlined in the second code snippet.

[tunnckoCore]

@NoelAbrahams yep, something like #635 looks beautiful and modern.

[NoelAbrahams]

@tunnckoCore I'm going to close this issue, as it was really more of a question, and there doesn't seem to be anything to fix.