fix http concurrency #4218
Merged
fix http concurrency #4218
+787
−204
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add tests handle edge cases and add unit tets update tests info add integ tests stack-info: PR: #4218, branch: GarrettBeatty/gcbeatty/taskoptimization/1
GarrettBeatty
force-pushed
the
GarrettBeatty/gcbeatty/taskoptimization/1
branch
from
610a889 to
91abd13
Compare
This was referenced Dec 10, 2025
Merged
Merged
Closed
GarrettBeatty
commented
Dec 10, 2025
| } | ||
| } | ||
|
|
||
| /// <summary> |
Contributor
Author
There was a problem hiding this comment.
i think we should eventually refactor this to get rid of the extra discoverdownloadstrategy function. since its a little confusing that the semaphore releases go across functions (between discoverdownloadstrategy and startdownload). i plan on doing that after we fix all of the bugs
GarrettBeatty
commented
Dec 10, 2025
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task DiscoverUsingPartStrategy_AcquiresAndReleasesHttpSlot() |
Contributor
Author
There was a problem hiding this comment.
these are no longer applicable since they asserted old (and incorrect behavior)
GarrettBeatty
commented
Dec 10, 2025
| // Act - Start both discoveries concurrently | ||
| var task1 = coordinator1.DiscoverDownloadStrategyAsync(CancellationToken.None); | ||
| var task2 = coordinator2.DiscoverDownloadStrategyAsync(CancellationToken.None); | ||
| var discovery1 = await coordinator1.DiscoverDownloadStrategyAsync(CancellationToken.None); |
Contributor
Author
There was a problem hiding this comment.
had to update these because now with new change the semaphore is held through start download, so callers need to do discover + start download in order to get through the whole http cycle
philasmar
approved these changes
Dec 10, 2025
normj
approved these changes
Dec 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Stacked PRs:
Description
Fix HTTP concurrency for multi part download. Previously we were releasing the http semaphore slot after we got the http response but before we read the response body (wrote it to a file/stream). This meant that it was possible for there to be more active
ConcurrentServiceRequeststhan the user actually wanted.The fix was to move the releasing of the http semaphore slot after processing the part. This ensured that its only released once its written to the file/stream and ensures only
ConcurrentServiceRequestsare active at a time.Motivation and Context
#3806
Testing
HttpSemaphore_HeldThroughProcessPartAsyncwhich verify that the http semaphore is only released after process part is executed. In order to ensure this test actually checks the issue, i ran it before my fix and the test failed (as expected). After my fix, the test passes.Screenshots (if appropriate)
Before (we can see concurrency is over the set 10)
After, we can see concurrency stays at or below 10
Types of changes
Checklist
License