-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue #1883, MultipartMixed grows cache indefinitely #1887
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,12 +56,12 @@ internal static class MediaTypeUtils | |
/// <summary> | ||
/// Max size to cache match info. | ||
/// </summary> | ||
private const int MatchInfoCacheMaxSize = 256; | ||
private const int MatchInfoCacheInitialSize = 256; | ||
|
||
/// <summary> | ||
/// Concurrent cache to cache match info. | ||
/// </summary> | ||
private static MatchInfoConcurrentCache MatchInfoCache = new MatchInfoConcurrentCache(MatchInfoCacheMaxSize); | ||
private static MatchInfoConcurrentCache MatchInfoCache = new MatchInfoConcurrentCache(MatchInfoCacheInitialSize); | ||
|
||
/// <summary>UTF-8 encoding, without the BOM preamble.</summary> | ||
/// <remarks> | ||
|
@@ -413,6 +413,15 @@ internal static ODataFormat GetFormatFromContentType(string contentTypeName, ODa | |
throw new ODataContentTypeException(Strings.MediaTypeUtils_CannotDetermineFormatFromContentType(str, contentTypeName)); | ||
} | ||
|
||
/// <summary> | ||
/// Internal method for testing membership of the cache | ||
/// </summary> | ||
/// <returns>The ContentTypes in the cache in the MediaInfoCache</returns> | ||
internal static IEnumerable<string> GetCacheKeys() | ||
{ | ||
return MatchInfoCache.GetKeys(); | ||
} | ||
|
||
/// <summary> | ||
/// Parses the specified content type header into a media type instance. | ||
/// </summary> | ||
|
@@ -908,7 +917,7 @@ public MatchInfoCacheKey(ODataMediaTypeResolver resolver, ODataPayloadKind paylo | |
{ | ||
this.MediaTypeResolver = resolver; | ||
this.PayloadKind = payloadKind; | ||
this.ContentTypeName = contentTypeName; | ||
this.ContentTypeName = RemoveBoundary(contentTypeName); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -924,7 +933,7 @@ public MatchInfoCacheKey(ODataMediaTypeResolver resolver, ODataPayloadKind paylo | |
/// <summary> | ||
/// Name of content type. | ||
/// </summary> | ||
private string ContentTypeName { get; set; } | ||
internal string ContentTypeName { get; set; } | ||
|
||
/// <summary> | ||
/// Returns a value indicating whether this instance is equal to a specified object. | ||
|
@@ -967,6 +976,27 @@ public override int GetHashCode() | |
int result = this.MediaTypeResolver.GetHashCode() ^ this.PayloadKind.GetHashCode(); | ||
return this.ContentTypeName != null ? result ^ this.ContentTypeName.GetHashCode() : result; | ||
} | ||
|
||
/// <summary> | ||
/// Multipart/mixed content types have a boundary that varies by request. | ||
/// It should not be used as part of the format key. | ||
/// </summary> | ||
/// <param name="contentTypeName"></param> | ||
/// <returns></returns> | ||
private static string RemoveBoundary(string contentTypeName) | ||
{ | ||
if (contentTypeName.StartsWith("multipart", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
{ | ||
return contentTypeName.Substring(0, parameter); | ||
} | ||
} | ||
|
||
return contentTypeName; | ||
} | ||
} | ||
|
||
/// <summary> | ||
|
@@ -982,10 +1012,10 @@ private sealed class MatchInfoConcurrentCache | |
/// <summary> | ||
/// Constructor. | ||
/// </summary> | ||
/// <param name="maxSize">Max size of the elements that the cache can contain.</param> | ||
public MatchInfoConcurrentCache(int maxSize) | ||
/// <param name="initialSize">Initial size of the cache.</param> | ||
public MatchInfoConcurrentCache(int initialSize) | ||
{ | ||
this.dict = new ConcurrentDictionary<MatchInfoCacheKey, MediaTypeMatchInfo>(4, maxSize); | ||
this.dict = new ConcurrentDictionary<MatchInfoCacheKey, MediaTypeMatchInfo>(4, initialSize); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -1018,6 +1048,15 @@ public void Add(MatchInfoCacheKey key, MediaTypeMatchInfo value) | |
this.dict.TryAdd(key, value); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Internal method for validating expected elements in the cache | ||
/// </summary> | ||
/// <returns>ContentType of items in the cache</returns> | ||
internal IEnumerable<string> GetKeys() | ||
{ | ||
return this.dict.Keys.Select(k => k.ContentTypeName); | ||
} | ||
} | ||
} | ||
} |
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json batch also has the "mulitpart"? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)