Fix issue #1883, MultipartMixed grows cache indefinitely #1887
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
… exploding the cache.
mikepizzo
force-pushed
the
MultipartMediaTypePerfFix
branch
from
d41d2ee to
103c685
Compare
mikepizzo
force-pushed
the
MultipartMediaTypePerfFix
branch
from
103c685 to
f0a6531
Compare
mikepizzo
added
P1
performance
Ready for review
xuzhg
reviewed
Sep 22, 2020
| /// <returns></returns> | ||
| private static string RemoveBoundary(string contentTypeName) | ||
| { | ||
| if (contentTypeName.StartsWith("multipart", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
json batch also has the "mulitpart"? #Resolved
There was a problem hiding this comment.
No; json uses application/json, which does not have a boundary parameter.
The boundary issue is specific to multipart/mixed, which uses a "boundary" mime type parameter to distinguish requests within the multipart/mixed batch payload.
In reply to: 492449803 [](ancestors = 492449803)
Sreejithpin
reviewed
Sep 22, 2020
| { | ||
| // our formatters don't care about parameters for multipart, so just strip them all out for simplicity | ||
| int parameter = contentTypeName.IndexOf(';'); | ||
| if (parameter > 0) |
There was a problem hiding this comment.
I assume there will be only 1 ';' in the string, so rather than index and then substring, can we do in 1 pass? may be by using a stringbuilder
xuzhg
approved these changes
Sep 22, 2020
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.
Issues
This pull request fixes issue #1883
Description
The process of matching a formatter to a media type can be expensive, so we cache the results of a lookup in our MediaTypeCache. Unfortunately, since the key for this cache is the content type, each new content type is a separate entry in the cache.
The multipart/mixed mime type, used for batch, has a required boundary parameter, expressed as part of the media type in the content-type header, which may be set to any random value.
Some clients have fixed values for these, but it's not uncommon to use a guid.
Because a guid makes each content-type unique, the cache size grows unbounded.
The boundary isn't needed to look up the formatter, so can safely be removed from the cache lookup key.
Also fixed apparent misunderstanding in the code of the size parameter passed to the concurrent dictionary constructor. It is not a max size, but an initial size for the cache.
Checklist (Uncheck if it is not completed)