-
Notifications
You must be signed in to change notification settings - Fork 39
Conversation
@@ -45,11 +45,17 @@ public void Remove(string key) | |||
} | |||
} | |||
|
|||
public void Set(string key, object entry) | |||
public void Set(string key, object entry, TimeSpan validUntil) |
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.
validUntil
is a strange name for a TimeSpan
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.
validFor
?
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.
yeah, or expireAfter
So far so good. |
5cfc0da
to
fe6ef55
Compare
{ | ||
var client = server.CreateClient(); | ||
var initialResponse = await client.GetAsync(""); | ||
client.DefaultRequestHeaders.CacheControl = new System.Net.Http.Headers.CacheControlHeaderValue() |
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.
@Tratcher is there any reason we have our own version of CacheControlHeaderValue
? It looks very similar to the one in System.Net.Http.Headers
.
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.
Yeah, we cloned most of System.Net.Http.Headers in order to make out-of-band usability and perf improvements.
namespace Microsoft.AspNetCore.ResponseCaching | ||
{ | ||
public interface IResponseCache | ||
{ | ||
object Get(string key); | ||
// TODO: Set expiry policy in the underlying cache? | ||
void Set(string key, object entry); | ||
void Set(string key, object entry, TimeSpan validFor); |
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.
We might need a context object here. What other things will we need to pass?
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.
What do we need in the context object? Should we add that when we actually need it?
ping @davidfowl @Tratcher |
|
||
if (body.Length > 0) | ||
{ | ||
await response.Body.WriteAsync(body, 0, body.Length).SupressContext(); |
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.
get rid of SuppresContext()
.
// Finalize the cache entry | ||
cachingContext.FinalizeCaching(); | ||
cachingContext.FinalizeCachingBody(); | ||
} | ||
finally |
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.
catch { disable response caching; throw; }
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 need, response caching is only completed if everything succeeds. Early exit by default behaves as if caching was disabled.
b57dcbe
to
dc4212c
Compare
public async Task SendFileAsync(string path, long offset, long? length, CancellationToken cancellation) | ||
{ | ||
_responseCacheStream.DisableBuffering(); | ||
await _originalSendFileFeature.SendFileAsync(path, offset, length, cancellation); |
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 async/await required.
|
4e732b9
to
62aabc1
Compare
todos:
cc @Tratcher @davidfowl