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(cursor): make eachAsync() avoid modifying batch when mixing parallel and batchSize #12716

Merged
merged 1 commit into from Nov 23, 2022

Conversation

vkarpov15
Copy link
Collaborator

Fix #12652

Summary

#12652 points out that there is a race condition in eachAsync() when parallel > 1 and batchSize is set. Given that we run a bunch of parallel queues, documentsBatch can change while the eachAsync callback is running.

This fixes part of the issue: makes it so that at least every document gets processed, and each eachAsync() callback gets a consistent batch of documents that doesn't change.

However, there is still the potential issue that the individual documents in the batch can be in any order when parallel > 1. Because we run next() in parallel, you can end up with surprising behavior like document 9613 in the same batch as documents 9900-9999. Or an eachAsync() callback running with a batch of size 67 followed by a batch of 33 even though you have batchSize = 100 and there's 10000 documents in the result set.

We should consider running next() in a queue, getting documents in order and building up the batch, and then building the next batch while eachAsync() runs. Rather than running the whole "build up a batch and then call the eachAsync() callback" process in parallel. What do you think @hasezoey ?

Examples

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

LGTM, though i am not really familiar with that part of the code or the functions of it.

Rather than running the whole "build up a batch and then call the eachAsync() callback" process in parallel. What do you think @hasezoey ?

if i understand correctly, currently it works "when soonest ready" and you want to change it "to always be when the batch size is fully in buffer"?

@vkarpov15
Copy link
Collaborator Author

@hasezoey currently it kicks off a separate "build the batch and call the callback" function in parallel. The problem is that "build the batch" can lead to inconsistent batches because of race conditions - every document will be processed once, but you can end up with batches with out-of-order documents. What I'm suggesting is instead run 1 "build the batch" function, regardless of parallel, and keep running "build the batch" while we still have fewer than parallel callbacks running.

MongoDB doesn't support multiple next() calls on a cursor in parallel anyway, so there's no performance benefit to having multiple "build the batch" functions running at the same time. We implemented it this way purely for expediency.

@vkarpov15 vkarpov15 merged commit 966eebe into master Nov 23, 2022
@hasezoey hasezoey deleted the vkarpov15/gh-12652 branch November 23, 2022 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants