fix(csharp): key CloudFetchDownloader.downloadTasks by ChunkIndex#436
Open
msrathore-db wants to merge 1 commit into
Open
fix(csharp): key CloudFetchDownloader.downloadTasks by ChunkIndex#436msrathore-db wants to merge 1 commit into
msrathore-db wants to merge 1 commit into
Conversation
The bookkeeping dict was declared as ConcurrentDictionary<Task, IDownloadResult>
and populated with `downloadTasks[downloadTask] = downloadResult` where
downloadTask is the continuation returned by DownloadFileAsync(...).ContinueWith(...).
Inside that continuation, however, the lambda's `t` parameter is the ANTECEDENT
task (the DownloadFileAsync task itself), not the continuation. So
`downloadTasks.TryRemove(t, ...)` never matched any key and entries accumulated
for the lifetime of DownloadFilesAsync. The dict is local and GC'd at method
exit so there's no observable leak, but the code lies about what it tracks
("active" downloads vs. "every download seen") and Task.WhenAll at the end-of-
results guard awaits already-completed tasks needlessly.
Switch the key to downloadResult.ChunkIndex (a long that's already unique per
chunk in a query) so the continuation's TryRemove uses the same key the add
used. Update Task.WhenAll to use .Values.
The IDownloadResult value was never read out of the dict — every site either
ignored the value or wrote it — so dropping it is safe.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Re-keys the active-downloads bookkeeping dictionary in
CloudFetchDownloader.DownloadFilesAsyncfrom<Task, IDownloadResult>to<long, Task>so the continuation'sTryRemoveactually removes its own entry.Background
The dict was declared as:
…and populated with
downloadTasks[downloadTask] = downloadResult, wheredownloadTaskis the continuation returned byDownloadFileAsync(...).ContinueWith(t => { ... }).Inside the continuation lambda, however,
tis the antecedent task (theDownloadFileAsynctask itself), not the continuation. So:Entries are never removed. The dict grows entry-by-entry for every chunk in a query and is only freed when
DownloadFilesAsyncreturns (the variable goes out of scope).Task.WhenAll(downloadTasks.Keys)at the end-of-results guard ends up awaiting every download started in the query, including the ones that finished hours earlier — harmless because awaiting completed tasks is essentially free, but wasteful and the code's claim that the dict tracks active downloads is wrong.Fix
downloadResult.ChunkIndex(along, already unique per chunk in a query).chunkIndexbefore the lambda so the closure can use it forTryRemove.Task.WhenAllto use.Values(the tasks now live as values, not keys).IDownloadResultvalue type — verified by reading every reference that the value was never read out of the dict (only assigned, removed without being read, or ignored viaout _); theIDownloadResultreferenced inside the continuation comes from the foreach loop variable via closure capture, not from a dict lookup.Why a separate PR
This came up during review of #183 (straggler download mitigation). It's not straggler-related — the bug pre-existed in the generic CloudFetch download loop. Splitting it out so it can be reviewed on its own merits.
Test plan
dotnet buildsucceeds with 0 warnings, 0 errorsdotnet test --filter "FullyQualifiedName~Unit")This pull request and its description were written by Isaac.