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

fix: prevent 'Maximum call stack size exceeded' error #204

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

Conversation

robertsLando
Copy link
Contributor

On very high loads mqemitter _do could throw Maximum call stack size exceeded error, using setImmediate to allow nodejs to clear the stack fixes it

@robertsLando
Copy link
Contributor Author

cc @mcollina

this._parallel(this, matches, message, callback)
setImmediate(
this._parallel.bind(this, this, matches, message, callback)
)
Copy link
Owner

Choose a reason for hiding this comment

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

Adding a setImmediate here will definitely cripple the performance of this module. A different solution is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions? process.nextTick ?

Copy link
Owner

Choose a reason for hiding this comment

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

Something way more convoluted. We should test if this was called in the same tick more than x times and then use setImmediate just in that case.

@mcollina
Copy link
Owner

Can you please add a test where this crash happens?

@robertsLando
Copy link
Contributor Author

robertsLando commented Dec 23, 2022

Can you please add a test where this crash happens?

I will try, I made it happen while using aedes on very high loads. Could I even do it in a separete file? I mean one that doesn't run with the test suite

@mcollina
Copy link
Owner

anything works, we can embed in the test suite later

@robertsLando
Copy link
Contributor Author

@mcollina I dunno why but I cannot reproduce this anymore in my repo even in the benchmark with high loads, keep this open for now if you want, will do some more tests in next days

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.

None yet

2 participants