Skip to content

fix: handle two busboy error events #1296

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

Open
wants to merge 2 commits into
base: lts
Choose a base branch
from

Conversation

mhassan1
Copy link

This PR enables multer to handle two out-of-band error events from busboy; it does this by replacing process.nextTick (which happens too soon after the first error event) with setImmediate.

I don't know if this fix will handle more than two, but I also don't think busboy will ever emit more than two.

This is a follow-up to #1177.

Resolves #1176.

@mhassan1
Copy link
Author

@UlisesGascon @LinusU Please take a look, when you have a chance. Thanks.

@LinusU
Copy link
Member

LinusU commented Apr 22, 2025

Could you duplicate the test into a new one, instead of modifying the current one, so that we are sure that the previous single error is also handled? 🙏

@mhassan1 mhassan1 force-pushed the two-busboy-error-events branch from b3d69ae to 8f6279b Compare April 22, 2025 13:50
@mhassan1
Copy link
Author

Could you duplicate the test into a new one, instead of modifying the current one, so that we are sure that the previous single error is also handled? 🙏

@LinusU Done, please take a look.

@mhassan1
Copy link
Author

mhassan1 commented May 6, 2025

@UlisesGascon @LinusU Please take a look, when you have a chance. Thanks.

@UlisesGascon UlisesGascon requested a review from LinusU May 9, 2025 13:09
@UlisesGascon
Copy link
Member

Not sure why the CI pipeline was not triggered 🤔

@bjohansebas
Copy link
Member

@UlisesGascon see #1302

@mhassan1
Copy link
Author

@UlisesGascon @LinusU Are the CI failures blockers to this PR? The tests are passing on supported Node.js versions.

@ctcpip
Copy link
Member

ctcpip commented May 16, 2025

this looks good, but we are pulling in this fix as part of another PR. I will update when that gets merged. thank you!

Copy link
Member

@ctcpip ctcpip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waiting to merge, please hold

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants