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: add isNaN check for concurrency limit #787

Merged
merged 2 commits into from
Jun 25, 2020
Merged

Conversation

WaleedAshraf
Copy link
Contributor

@WaleedAshraf WaleedAshraf commented Jun 25, 2020

Closes #788

@WaleedAshraf WaleedAshraf marked this pull request as ready for review June 25, 2020 02:34
@ankon
Copy link
Contributor

ankon commented Jun 25, 2020

Out of interest: How did you manage to get this to be NaN in the first place? I thinking there's probably more places that don't specifically look at NaN in the code base because there's no reasonable path to get into that state.

@Nevon
Copy link
Collaborator

Nevon commented Jun 25, 2020

I'm generally not a big fan of these runtime type checks when there isn't a reasonable way that you'd expect to possibly receive weird values, but given that it's already checking that the limit is a number I supposed there's no harm in also checking that it's not NaN (love that NaN is a number and also neither equal to, larger or smaller than 1). But yeah, how did you get it to be NaN to begin with? 😃

@WaleedAshraf
Copy link
Contributor Author

Before we pass config values to kafkajs, we do parseInt. And, There is WTF JS 😆

Number.parseInt(undefined) // NaN

@Nevon Nevon merged commit d4ee12f into tulios:master Jun 25, 2020
@WaleedAshraf WaleedAshraf deleted the fix-nan branch June 25, 2020 14:44
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.

Consumer hangs if concurrency is NaN
3 participants