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

urgent: fix for drain event #118

Merged
merged 5 commits into from Jul 6, 2011
Merged

urgent: fix for drain event #118

merged 5 commits into from Jul 6, 2011

Conversation

dvv
Copy link
Contributor

@dvv dvv commented Jul 1, 2011

please, consider

@dvv
Copy link
Contributor Author

dvv commented Jul 2, 2011

https://github.com/dvv/node_redis/commit/9a4e51ee4011ed2fa1ac5d28e1fc6e341987a2ed is critical

https://github.com/dvv/node_redis/blob/master/index.js#L284 -- we are adding Boolean to number, then check for strict equality later at https://github.com/dvv/node_redis/blob/master/index.js#L289. Such condition never holds unless offline_queue is trivially empty. This results in drain event fire once/twice and no more.

@mranney
Copy link
Contributor

mranney commented Jul 6, 2011

I've tested this drain logic against multiple different servers and link speeds, and it worked fine. Line 284 coerces the boolean to a number by incrementing it. I'll admit this is a tad awkward, but it seems sound. Is there a case where this doesn't work?

> var a = 0
> a += !true
0
> a === 0
true

@dvv
Copy link
Contributor Author

dvv commented Jul 6, 2011

Sure. Please, run stress tests from my fork. If they worked, I'd never look into L284 ;)
In your examples the number of iterations too small (10000). Make it forever cycle, and things should reveal.

mranney added a commit that referenced this pull request Jul 6, 2011
@mranney mranney merged commit 4930036 into redis:master Jul 6, 2011
@mranney
Copy link
Contributor

mranney commented Jul 6, 2011

I can't reproduce the failure, but the falsiness test is more consistent, so we might as well do that.

Thanks for looking into this.

@mranney
Copy link
Contributor

mranney commented Jul 6, 2011

Pushed v0.6.5 with your change.

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