Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement parallel downloads for COM scenarios #1588
Implement parallel downloads for COM scenarios #1588
Changes from 2 commits
6db5e2e
185f8ea
29c98fc
f082198
1f5de22
303e86e
fa1df2f
4dbedee
43a53b1
cb18b79
2e35ce4
cc9546e
0e34dd7
0c4ca56
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pre-existing issue, but there is the potential for Id collisions from different sources here. Not an immediate concern but we might want to consider how we can store them all in the same index, or use a separate index per source. Most options are probably annoying and relatively expensive to implement, and the current potential for problems is low. Just leaving this here as a reminder to you and me both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably be keeping the
std::thread
objects because I don't think that keeping tracking of running threads with a counter will work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically I think that the use of a counter here is fragile; if the thread terminates abnormally it won't decrement. With proper handling you can make it safer and more robust, but you could also put that effort into using the kernel to know the answer. And it has just a bit more mileage than any code we write here 😄
So I would write a thread wrapper type and hold onto them. You can
WaitForSingleObject
on the native handle (probably) as suggested https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getexitcodethreadThat way we could say for a fact how many threads were still running at this point.