Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Request CachePolicy isn't applied in HTTP request header #60913

Merged
merged 7 commits into from
Nov 9, 2021
Merged

Request CachePolicy isn't applied in HTTP request header #60913

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
59b5a15
Request CachePolicy isn't applied in HTTP request header
pedrobsaila Oct 27, 2021
3d4bf86
handle other CachePolicy levels
pedrobsaila Oct 27, 2021
e272a65
manage max-stale cache policy case
pedrobsaila Oct 27, 2021
9ed19f3
handle DefaultCachePolicy
pedrobsaila Nov 2, 2021
ac36aec
CachePolicy is nullable by default
pedrobsaila Nov 3, 2021
32dffd6
explicitely set DefaultCachePolicy and DefaultCachePolicy tests execu…
pedrobsaila Nov 6, 2021
f499323
fix minor issues
pedrobsaila Nov 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.IO;
using System.Net.Cache;
using System.Net.Http;
using System.Net.Http.Headers;
using System.Net.Security;
using System.Net.Sockets;
using System.Runtime.Serialization;
Expand Down Expand Up @@ -1137,6 +1138,8 @@ private async Task<WebResponse> SendRequest(bool async)
request.Headers.Host = Host;
}

AddCacheControlHeaders(request);

// Copy the HttpWebRequest request headers from the WebHeaderCollection into HttpRequestMessage.Headers and
// HttpRequestMessage.Content.Headers.
foreach (string headerName in _webHeaderCollection)
Expand Down Expand Up @@ -1202,6 +1205,100 @@ private async Task<WebResponse> SendRequest(bool async)
}
}

private void AddCacheControlHeaders(HttpRequestMessage request)
{
RequestCachePolicy? policy = GetApplicableCachePolicy();

if (policy != null)
{
if (request.Headers.CacheControl == null)
{
request.Headers.CacheControl = new CacheControlHeaderValue();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CacheControl will always be null here. I would:

var cacheControl = new CacheControlHeaderValue();

And accessing request.Headers.CacheControl over and over will do non-trivial work, so I'd use this local variable in the switch body and then after setting everything:

request.Headers.CacheControl = cacheControl;


if (policy is HttpRequestCachePolicy httpRequestCachePolicy)
{
switch (httpRequestCachePolicy.Level)
{
case HttpRequestCacheLevel.NoCacheNoStore:
request.Headers.CacheControl.NoCache = true;
request.Headers.CacheControl.NoStore = true;
request.Headers.Pragma.Add(new NameValueHeaderValue("no-cache"));
break;
case HttpRequestCacheLevel.Reload:
request.Headers.CacheControl.NoCache = true;
request.Headers.Pragma.Add(new NameValueHeaderValue("no-cache"));
break;
case HttpRequestCacheLevel.CacheOnly:
scalablecory marked this conversation as resolved.
Show resolved Hide resolved
case HttpRequestCacheLevel.CacheOrNextCacheOnly:
request.Headers.CacheControl.OnlyIfCached = true;
break;
case HttpRequestCacheLevel.Default:
if (httpRequestCachePolicy.MinFresh > TimeSpan.Zero)
{
request.Headers.CacheControl.MinFresh = httpRequestCachePolicy.MinFresh;
}

if (httpRequestCachePolicy.MaxAge != TimeSpan.MaxValue)
{
request.Headers.CacheControl.MaxAge = httpRequestCachePolicy.MaxAge;
}

if (httpRequestCachePolicy.MaxStale > TimeSpan.Zero)
{
request.Headers.CacheControl.MaxStale = true;
request.Headers.CacheControl.MaxStaleLimit = httpRequestCachePolicy.MaxStale;
}

break;
case HttpRequestCacheLevel.Refresh:
request.Headers.CacheControl.MaxAge = TimeSpan.Zero;
request.Headers.Pragma.Add(new NameValueHeaderValue("no-cache"));
break;
}
}
else
{
switch (policy.Level)
{
case RequestCacheLevel.NoCacheNoStore:
request.Headers.CacheControl.NoCache = true;
request.Headers.CacheControl.NoStore = true;
request.Headers.Pragma.Add(new NameValueHeaderValue("no-cache"));
break;
case RequestCacheLevel.Reload:
request.Headers.CacheControl.NoCache = true;
request.Headers.Pragma.Add(new NameValueHeaderValue("no-cache"));
break;
case RequestCacheLevel.CacheOnly:
request.Headers.CacheControl.OnlyIfCached = true;
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CacheOnly is intended to throw if it's not in the local cache already. This is different from OnlyIfCached, which is a directive to the server/proxy.

.NET no longer has a local cache, so this can always throw a WebException. The text used in Framework is "The request was aborted: The request cache-only policy does not allow a network request and the response is not found in cache." and should be added as a resource string.

}
}
}
}

private RequestCachePolicy? GetApplicableCachePolicy()
{
if (CachePolicy != null)
{
return CachePolicy;
}
else if (IsDefaultCachePolicySet(DefaultCachePolicy))
{
return DefaultCachePolicy;
}
else if (IsDefaultCachePolicySet(WebRequest.DefaultCachePolicy))
{
return WebRequest.DefaultCachePolicy;
}

return null;
}

private static bool IsDefaultCachePolicySet(RequestCachePolicy? policy) => policy != null
&& policy.Level != RequestCacheLevel.BypassCache;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem correct: if I explicitly want my request to bypass cache, this will end up using the default policy (which might be to cache).

I think correct behavior is to stop at the first non-null policy, and return null if that policy says bypass.

Copy link
Contributor

@scalablecory scalablecory Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, can you make sure BypassCache doesn't need to do anything to the request?

The docs say it bypasses all caches "between client and server", and it's not clear if this needs to set some header related to proxies.

Copy link
Contributor Author

@pedrobsaila pedrobsaila Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem correct: if I explicitly want my request to bypass cache, this will end up using the default policy (which might be to cache).

I think correct behavior is to stop at the first non-null policy, and return null if that policy says bypass.

I did it because the DefaultCachePolicy is initialized with BypassCache in WebRequest and HttpWebRequest classes by default. if a user sets WebRequest.DefaultCachePolicy and didn't do it for HttpWebRequest.DefaultCachePolicy we will presume that no cache gonna be applied, which may seem misleading for the user

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it because the DefaultCachePolicy is initialized with BypassCache in WebRequest and HttpWebRequest classes by default. if a user sets WebRequest.DefaultCachePolicy and didn't do it for HttpWebRequest.DefaultCachePolicy we will presume that no cache gonna be applied, which may seem misleading for the user

It sounds like we need to update those properties to track "explicitly set" vs "default" state. Pull the first one that is explicitly set.


public override IAsyncResult BeginGetResponse(AsyncCallback? callback, object? state)
{
CheckAbort();
Expand Down
98 changes: 97 additions & 1 deletion src/libraries/System.Net.Requests/tests/HttpWebRequestTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,6 @@ public void Ctor_VerifyDefaults_Success(Uri remoteServer)
Assert.Equal("GET", request.Method);
Assert.Equal(64, HttpWebRequest.DefaultMaximumResponseHeadersLength);
Assert.NotNull(HttpWebRequest.DefaultCachePolicy);
Assert.Equal(RequestCacheLevel.BypassCache, HttpWebRequest.DefaultCachePolicy.Level);
Assert.Equal(0, HttpWebRequest.DefaultMaximumErrorResponseLength);
Assert.NotNull(request.Proxy);
Assert.Equal(remoteServer, request.RequestUri);
Expand Down Expand Up @@ -1924,6 +1923,103 @@ public void Abort_CreateRequestThenAbort_Success(Uri remoteServer)
request.Abort();
}

[Theory]
[InlineData(HttpRequestCacheLevel.NoCacheNoStore, null, null, new string[] { "Pragma: no-cache", "Cache-Control: no-store, no-cache"})]
[InlineData(HttpRequestCacheLevel.Reload, null, null, new string[] { "Pragma: no-cache", "Cache-Control: no-cache" })]
[InlineData(HttpRequestCacheLevel.CacheOnly, null, null, new string[] { "Cache-Control: only-if-cached" })]
[InlineData(HttpRequestCacheLevel.CacheOrNextCacheOnly, null, null, new string[] { "Cache-Control: only-if-cached" })]
[InlineData(HttpRequestCacheLevel.Default, HttpCacheAgeControl.MinFresh, 10, new string[] { "Cache-Control: min-fresh=10" })]
[InlineData(HttpRequestCacheLevel.Default, HttpCacheAgeControl.MaxAge, 10, new string[] { "Cache-Control: max-age=10" })]
[InlineData(HttpRequestCacheLevel.Default, HttpCacheAgeControl.MaxStale, 10, new string[] { "Cache-Control: max-stale=10" })]
[InlineData(HttpRequestCacheLevel.Refresh, null, null, new string[] { "Pragma: no-cache", "Cache-Control: max-age=0" })]
public async Task SendHttpGetRequest_WithHttpCachePolicy_AddCacheHeaders(
HttpRequestCacheLevel requestCacheLevel, HttpCacheAgeControl? ageControl, int? age, string[] expectedHeaders)
{
await LoopbackServer.CreateServerAsync(async (server, uri) =>
{
HttpWebRequest request = WebRequest.CreateHttp(uri);
request.CachePolicy = ageControl != null ?
new HttpRequestCachePolicy(ageControl.Value, TimeSpan.FromSeconds((double)age))
: new HttpRequestCachePolicy(requestCacheLevel);
Task<WebResponse> getResponse = GetResponseAsync(request);

await server.AcceptConnectionAsync(async connection =>
{
List<string> headers = await connection.ReadRequestHeaderAndSendResponseAsync();

foreach (string header in expectedHeaders)
{
Assert.Contains(header, headers);
}
});

using (var response = (HttpWebResponse)await getResponse)
{
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}
});
}

[Theory]
[InlineData(RequestCacheLevel.NoCacheNoStore, new string[] { "Pragma: no-cache", "Cache-Control: no-store, no-cache" })]
[InlineData(RequestCacheLevel.Reload, new string[] { "Pragma: no-cache", "Cache-Control: no-cache" })]
[InlineData(RequestCacheLevel.CacheOnly, new string[] { "Cache-Control: only-if-cached" })]
public async Task SendHttpGetRequest_WithCachePolicy_AddCacheHeaders(
RequestCacheLevel requestCacheLevel, string[] expectedHeaders)
{
await LoopbackServer.CreateServerAsync(async (server, uri) =>
{
HttpWebRequest request = WebRequest.CreateHttp(uri);
request.CachePolicy = new RequestCachePolicy(requestCacheLevel);
Task<WebResponse> getResponse = GetResponseAsync(request);

await server.AcceptConnectionAsync(async connection =>
{
List<string> headers = await connection.ReadRequestHeaderAndSendResponseAsync();

foreach (string header in expectedHeaders)
{
Assert.Contains(header, headers);
}
});

using (var response = (HttpWebResponse)await getResponse)
{
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}
});
}

[Theory]
[InlineData(RequestCacheLevel.NoCacheNoStore, new string[] { "Pragma: no-cache", "Cache-Control: no-store, no-cache" })]
[InlineData(RequestCacheLevel.Reload, new string[] { "Pragma: no-cache", "Cache-Control: no-cache" })]
[InlineData(RequestCacheLevel.CacheOnly, new string[] { "Cache-Control: only-if-cached" })]
public async Task SendHttpGetRequest_WithGlobalCachePolicy_AddCacheHeaders(
RequestCacheLevel requestCacheLevel, string[] expectedHeaders)
{
await LoopbackServer.CreateServerAsync(async (server, uri) =>
{
HttpWebRequest.DefaultCachePolicy = new RequestCachePolicy(requestCacheLevel);
HttpWebRequest request = WebRequest.CreateHttp(uri);
Task<WebResponse> getResponse = GetResponseAsync(request);

await server.AcceptConnectionAsync(async connection =>
{
List<string> headers = await connection.ReadRequestHeaderAndSendResponseAsync();

foreach (string header in expectedHeaders)
{
Assert.Contains(header, headers);
}
});

using (var response = (HttpWebResponse)await getResponse)
{
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}
});
}

private void RequestStreamCallback(IAsyncResult asynchronousResult)
{
RequestState state = (RequestState)asynchronousResult.AsyncState;
Expand Down
34 changes: 34 additions & 0 deletions src/libraries/System.Net.Requests/tests/WebRequestTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Net.Cache;
using System.Net.Test.Common;
using System.Threading.Tasks;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;

Expand Down Expand Up @@ -186,6 +190,36 @@ public void RegisterPrefix_DuplicateHttpWithFakeFactory_ExpectFalse()
Assert.False(success);
}

[Theory]
[InlineData(RequestCacheLevel.NoCacheNoStore, new string[] { "Pragma: no-cache", "Cache-Control: no-store, no-cache" })]
[InlineData(RequestCacheLevel.Reload, new string[] { "Pragma: no-cache", "Cache-Control: no-cache" })]
[InlineData(RequestCacheLevel.CacheOnly, new string[] { "Cache-Control: only-if-cached" })]
public async Task SendGetRequest_WithGlobalCachePolicy_AddCacheHeaders(
RequestCacheLevel requestCacheLevel, string[] expectedHeaders)
{
await LoopbackServer.CreateServerAsync(async (server, uri) =>
{
WebRequest.DefaultCachePolicy = new RequestCachePolicy(requestCacheLevel);
WebRequest request = WebRequest.Create(uri);
Task<WebResponse> getResponse = request.GetResponseAsync();

await server.AcceptConnectionAsync(async connection =>
{
List<string> headers = await connection.ReadRequestHeaderAndSendResponseAsync();

foreach (string header in expectedHeaders)
{
Assert.Contains(header, headers);
}
});

using (var response = (HttpWebResponse)await getResponse)
{
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
}
});
}

private class FakeRequest : WebRequest
{
private readonly Uri _uri;
Expand Down