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(kv): Prevent infinite loop condition when listing tasks by org. #16249

Merged
merged 2 commits into from
Dec 18, 2019

Conversation

brettbuddin
Copy link
Contributor

Closes influxdata/idpe#5592

In the event that findTaskByIDWithAuth cannot find the task ID contained in the bucket, the outer loop will never terminate.

This ensures that we are calling Next() along-side any calls to continue while using a Cursor.

Notes:

I investigated improving the overall ergonomics of these cursor-based loops by hoisting the cursor advancement to the top of the iterations—instead of being at the tail-end of them. However, due to seeking behavior, the pre-conditions of the loop become a bit trickier. Perhaps this is an area of further improvement. Generally, loops that have cross-iteration state interaction tend to cause surprising behavior like this when continue gets involved.

Copy link
Contributor

@e-dard e-dard left a comment

Choose a reason for hiding this comment

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

Nice. It didn't seem like a deadlock, so this outcome makes a lot of sense.

In the event that findTaskByIDWithAuth cannot find the task ID contained
in the bucket, the outer loop will never terminate.

This ensures that we are calling Next() along-side any calls to continue
while using a Cursor.
@brettbuddin brettbuddin merged commit 2967462 into master Dec 18, 2019
@brettbuddin brettbuddin deleted the bb-infinite-loop branch December 18, 2019 15:06
@stuartcarnie
Copy link
Contributor

@brettbuddin 💯 for looking for ways to improve the loop ergonomics and your pragmatism for the urgency.

We could follow this up later with an improvement like this: https://play.golang.org/p/dygkrQYG2fu

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.

4 participants