fix(downloads): resolve a client's path mappings once per batch, not per item - #871
Open
m4bard wants to merge 1 commit into
Open
fix(downloads): resolve a client's path mappings once per batch, not per item#871m4bard wants to merge 1 commit into
m4bard wants to merge 1 commit into
Conversation
…per item DownloadClientGateway.GetQueueAsync fans out over every queue item, and each item translated its paths by calling IRemotePathMappingService.TranslatePathAsync, which queries the repository for that client's mappings on every call. The service, the repository and the ListenArrDbContext behind them are all scoped, so a queue of N items issued up to 2N concurrent queries against a context that permits one at a time. DownloadClientQueuePoller then runs that whole thing inside its own Task.WhenAll across every enabled client, so the fan-out is nested. This is the origin of the trace in upstream Listenarrs#783. The exception surfaces at DownloadClientQueuePoller.FetchAsync because that is where the await unwinds; the class itself holds no repository and no context and never did. Split the lookup from the translation. IRemotePathMappingService gains a TranslatePath overload that takes mappings the caller has already resolved and does no I/O, and TranslatePathAsync keeps its behaviour by fetching and delegating to it. The gateway resolves once per client before each fan-out and passes the result down. The single-item import path also resolves once, where it is one query either way. Asserting on the EF exception would mean racing it, so the test counts overlap directly: a stub records the highest number of lookups in flight. Ten items carrying two translatable paths each report 10 concurrent lookups without the change and 1 with it, and the lookup count is pinned at 1 so a regression fails even if it somehow avoids overlapping.
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
1 participant
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
DownloadClientGateway.GetQueueAsyncfans out over every queue item, and each item translated its paths by callingTranslatePathAsync, which queries the repository for that client's mappings on every call. The service, the repository and theListenArrDbContextbehind them are all scoped, so a queue of N items issued up to 2N concurrent queries against a context that permits one at a time.DownloadClientQueuePollerruns that inside its ownTask.WhenAllacross every client, so the fan-out is nested.Full write-up in #870.
Changes
Fixed
IRemotePathMappingServicegains aTranslatePathoverload taking mappings the caller has already resolved. It does no I/O.TranslatePathAsynckeeps its behaviour by fetching and delegating to it, so existing callers are unaffected.DownloadClientGatewayresolves a client's mappings once before each of its two item fan-outs and passes them down.Fewer queries as well as non-overlapping ones: a thirty-item queue goes from up to sixty to one.
On #783
That issue reports this exception class with a trace through
DownloadClientQueuePoller.FetchAsync. That class holds no repository and no context, and did not at4555ad21either, which is the commit it reports against. The trace surfaces there because that is where the await unwinds; the query is further down this chain.I have not reproduced #783's original scenario and this does not claim to close it.
Testing
DownloadClientGatewayPathMappingConcurrencyTestscounts the overlap directly rather than racing the EF exception, which would make for a flaky test and a weak claim. A stub records the highest number of lookups in flight, with a small delay so the overlap is observable rather than a scheduling accident.Ten items carrying two translatable paths each report 10 concurrent lookups without the change and 1 with it. The lookup count is also pinned at 1, so a change that restores per-item querying fails even if it somehow avoids overlapping.
Full suite: 3,030 passed, 0 failed, 125 skipped, against a 3,029 baseline on
03958c15.Note
This is the second instance of the same pattern, after #862 and its PR #863, which fixes a smaller one in search-result scoring. They are separate files and independently revertable, so they are separate PRs. I originally reported that scoring site as the only instance; that was wrong and is corrected on that issue.