Skip to content

Fix bug prunePendingRequestQueue returning empty list#14

Open
sudokien wants to merge 1 commit into
chuyskywalker:masterfrom
sudokien:prune_pending_request_bug
Open

Fix bug prunePendingRequestQueue returning empty list#14
sudokien wants to merge 1 commit into
chuyskywalker:masterfrom
sudokien:prune_pending_request_bug

Conversation

@sudokien
Copy link
Copy Markdown

Pull request for issue #13

@chuyskywalker
Copy link
Copy Markdown
Owner

I see what I've done wrong here, I did the decrement too late. The idea was that if you put the loop at zero, then it would never end until all the items have been accounted for by the break clause. This would work if the while was prefixed, ala while(--$limit), but that's wrong in most other cases.

The easier fix here would be to add

if ($limit <= 0) {
    $limit = -1;
}

right at the function head.

@sudokien
Copy link
Copy Markdown
Author

Hi chuyskywalker,

Yes you're totally right about that. But in my personal opinion I think setting $limit = -1 then having a while ($limit--) will be a bit hard to understand at first sight, since it's an infinite loop and we control iterations by break. If we have while ($countPending--) like in my PR, whoever read this line will know right away that this while loop will loop for every pending requests that haven't been processed. Just a matter of coding style, your fix definitely works (and is shorter).

Can we have this fix in the next release please? :)

Cheers,
Kien

@sudokien
Copy link
Copy Markdown
Author

sudokien commented Apr 7, 2015

Hi again,

Is there any chance to push this?

@nhatthm
Copy link
Copy Markdown

nhatthm commented Aug 11, 2015

Hi, do you have any updates on this request? I'm using this on production for a while and haven't seen any problems with it.

@nhatthm
Copy link
Copy Markdown

nhatthm commented Dec 30, 2015

Hello @chuyskywalker, do you have update on this PR? :) Thanks

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.

3 participants