Fix collection modified exception which causes rare HTTP 500s #5830
Conversation
This file contains 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
joelverhagen
requested review from
agr,
loic-sharma,
xavierdecoster,
shishirx34,
ryuyu,
dtivel,
chenriksson,
cristinamanum,
skofman1 and
scottbommarito
skofman1
reviewed
Apr 20, 2018
|
|
||
| while (jsonReader.Read()) | ||
| using (var jsonReader = new JsonTextReader(new StreamReader(blobStream))) |
There was a problem hiding this comment.
please make sure all the IDisposables will work well with each other (nothing missing, no double dispose that throws..)
There was a problem hiding this comment.
Verified.
There are three IDisposable instances in question here:
- The stream of the blob contents, returned by
CloudBlob.OpenRead(). This is wrapped in ausing. This is also passed toStreamReaderconstructor. StreamReader. This is passed toJsonTextReaderconstructor.JsonTextReader. This is wrapped in ausing.
IDisposable number 1 and 3 are in using blocks and number 2 is owned by JsonTextReader which, by default, disposes the provided TextReader.
In short, we're good.
Add unit tests to verify casing changes and repro of bug Use ConcurrentDictionary instead of Dictionary Address #5779
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.
Had some free time so I wanted to fix some intermittent 500s related to getting download counts on an ID. This PR addresses #5779.
Things done:
ConcurrentDictionaryin CloudDownloadCountService instead ofDictionary. This allows concurrent reads (via HTTP requests) and writes (via a background refresh every 15 minutes)StringComparer.OrdinalIgnoreCase. This should reduce allocations (not measured though).Dictionary.Performance analysis:
DictionaryRefreshDictionaryTryGetDownloadCountForPackageRegistrationConcurrentDictionaryRefreshConcurrentDictionaryTryGetDownloadCountForPackageRegistrationFor
Refresh, I copied thedownloads.v1.jsonfrom PROD to my own storage account and ran gallery code from my dev box. 100 iterations. Every 10 iterations I would refresh from scratch.For
TryGetDownloadCountForPackageRegistration, I ran all PROD IDs throughTryGetDownloadCountForPackageRegistration, which 176,157 invocations of the method. 245 iterations. Every 49 iterations I re-initialized the server (newRefresh).I thing the stability of
ConcurrentDictionaryis worth the minor performance decrease. We can consider an alternative approach if you guys think its worth it.