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

form.parse's Promise never resolves in tests #959

Open
Mitame opened this issue Nov 24, 2023 · 8 comments
Open

form.parse's Promise never resolves in tests #959

Mitame opened this issue Nov 24, 2023 · 8 comments
Labels

Comments

@Mitame
Copy link

Mitame commented Nov 24, 2023

Support plan

  • Which support plan is this issue covered by? (Community, Sponsor, Enterprise): Community
  • Currently blocking your project/work? (yes/no): no
  • Affecting a production system? (yes/no): no

Context

  • Node.js version: 18.18.2
  • Release Line of Formidable (Legacy, Current, Next): next
  • Formidable exact version: v3.5.1
  • Environment (node, browser, native, OS): Node 18.18.2, Linux (though likely all runtimes are affected)
  • Used with (popular names of modules): node-mocks-https, form-data

Note this issue appears to only be present in tests as it is dependent on how promises get queued, but it could lead to a bug in live code.

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

A (somewhat) minimal reproducible test.

import FormData from "form-data";
import formidable from "formidable";
import { createMocks } from "node-mocks-http";

test("Can parse files from node-mocks-http", async () => {
    let formData = new FormData();
    formData.append("file", "File data", "data.txt");

    const getLengthAsync: () => Promise<number> = async () => {
        return new Promise((res, rej) => {
            formData.getLength((err, length) => {
                if (err) {
                    rej(err);
                } else {
                    res(length);
                }
            });
        });
    };

    const { req, res } = createMocks({
        headers: {
          'content-type': `multipart/form-data;boundary="${formData.getBoundary()}"`,
          'content-length': (await getLengthAsync()).toString(),
        },
    });

    let form = formidable({});
    let promise = form.parse(req); 
    req.send(formData.getBuffer());
    let [fields, files] = await promise;
    console.log({fields, files});
})

What was the result you got?

The test times out at 5000ms because the await form.parse(req) never resolves.

What result did you expect?

The test passes.

Cause

The issue is caused as the callback handlers are set after an await of a promise.

await this.writeHeaders(req.headers);

This means the function returns, then data is sent which formidable is not listening for, then the await within parse is resolved and the handlers are set, but the events have already been missed, so the handlers just do nothing until Jest stops the test.

Workaround

It's possible to call req.send in a promise with a slight delay, e.g.

let promiseParse = form.parse(req);
let promiseSend = new Promise((resolve, reject) => {
    setTimeout(() => {
        req.send(formData.getBuffer());
        resolve(undefined);
    }, 50)
}
Promise.all([promiseParse, promiseSend]);

Breaking change

This is also a breaking change as it seems to have been introduced since v3.2.4. While the Promise style of parse didn't exist in that version, this bug also affects the callback style in the same manor.

@Mitame Mitame added the bug label Nov 24, 2023
@allthesignals
Copy link

This is happening to me too, not even in test environment. 3.5.1. The promise just never resolves

@Angryman18
Copy link

If you have body-parser package just remove or comment it. Hope it will resolve

@GrosSacASac
Copy link
Contributor

(reminder for me: try req.on('data') earlier)

are you testing formidable, or a project using formidable ?

@george-i
Copy link

If you have body-parser package just remove or comment it. Hope it will resolve

I tried commenting out busboy and busboy-body-parser and the parsing completed successfully.

@egargale
Copy link

If you have body-parser package just remove or comment it. Hope it will resolve

I had been banging my head against the wall for two days.... Thank you

@tunnckoCore
Copy link
Member

@george-i @egargale yeah, that's a known thing that things don't work well if someone else is interacting with the body too.

Formidable can be simplified A LOT, but the problem to move directly the newest stuff is that people are relying on old versions, there's still people on v1.. I had the code ready for over a year, a dream of mine for v4-5, but yeah ;d

@Ray0907
Copy link

Ray0907 commented Mar 11, 2024

I encounter the same issue. Is there any solution available to resolve the problem?

maxlath added a commit to inventaire/inventaire that referenced this issue Apr 23, 2024
This is addressing a critical error, see GHSA-8cp3-66vr-3r4c

That was not trivial as I bumped into this bug node-formidable/formidable#959
which led me to move form parsing to the middleware stage
@maxlath
Copy link

maxlath commented Apr 23, 2024

I migrated an Express app using body-parser and formidable v1.2.6 to formidable v3.5.1, and bumped into that issue. I was able to keep body-parser on, by calling await form.parse(req) from a middleware running before body-parser. See commit inventaire/inventaire@3b3c556. Hope that that can help others

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

9 participants