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

pedrobsaila · 2021-10-27T12:41:52Z

ghost · 2021-10-27T12:41:59Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Author:	pedrobsaila
Assignees:	-
Labels:	`area-System.Net`
Milestone:	-

pedrobsaila · 2021-10-27T14:33:45Z

I tried to do the same processing as in https://referencesource.microsoft.com/#System/net/System/Net/Cache/_Rfc2616CacheValidators.cs (Rfc2616.OnValidateRequest method)

scalablecory · 2021-11-01T19:39:14Z

How does this interact with the static HttpWebRequest.DefaultCachePolicy property?

pedrobsaila · 2021-11-02T15:25:51Z

How does this interact with the static HttpWebRequest.DefaultCachePolicy property?

Thanks for spotting that I almost missed it ;). In .NET 4.8, there's a class called RequestCacheManager that manages global CachePolicy configuration for HTTP/HTTPS or FTP. The rule is if a DefaultCachePolicy is set, it applies to all HTTP requests unless some specific HTTP request defines its own CachePolicy, then it's overridden by the last. The problem in .NET Core, .NET 5/6 is that there's no RequestCacheManager class so there's no way to apply it by default. In this actual state, DefaultCachePolicy is useless. What do you think @scalablecory : should i go for implementing RequestCacheManager ?

scalablecory · 2021-11-02T16:33:01Z

should i go for implementing RequestCacheManager ?

Judging by the Framework code, something like this (plus relevant tests) seems enough:

HttpRequestCachePolicy cachePolicy = request.CachePolicy ?? HttpWebRequest.DefaultCachePolicy ?? WebRequest.DefaultCachePolicy

scalablecory

thanks. some more comments.

scalablecory · 2021-11-03T21:50:38Z

src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs

+                if (request.Headers.CacheControl == null)
+                {
+                    request.Headers.CacheControl = new CacheControlHeaderValue();
+                }


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;

scalablecory · 2021-11-03T21:53:48Z

src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs

+        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;


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.

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.

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

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.

scalablecory · 2021-11-03T23:23:13Z

src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs

+                        case RequestCacheLevel.CacheOnly:
+                            request.Headers.CacheControl.OnlyIfCached = true;
+                            break;


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.

src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs

pedrobsaila · 2021-11-04T22:56:17Z

should i go for implementing RequestCacheManager ?

Judging by the Framework code, something like this (plus relevant tests) seems enough:
HttpRequestCachePolicy cachePolicy = request.CachePolicy ?? HttpWebRequest.DefaultCachePolicy ?? WebRequest.DefaultCachePolicy

It seems that adding test cases for DefaultCachePolicy broke other tests as it is static and tests are not executed sequentially, can I drop the test cases for DefaultCachePolicy ? or there's a way around this problem?

scalablecory · 2021-11-04T23:42:33Z

should i go for implementing RequestCacheManager ?

Judging by the Framework code, something like this (plus relevant tests) seems enough:
HttpRequestCachePolicy cachePolicy = request.CachePolicy ?? HttpWebRequest.DefaultCachePolicy ?? WebRequest.DefaultCachePolicy
It seems that adding test cases for DefaultCachePolicy broke other tests as it is static and tests are not executed sequentially, can I drop the test cases for DefaultCachePolicy ? or there's a way around this problem?

There's a class RemoteExecutor that we use for this, which will launch your test in a separate process.

…tion on side process

scalablecory

looking good, only some minor things.

scalablecory · 2021-11-08T17:46:09Z

src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs

+                            if (httpRequestCachePolicy.MinFresh > TimeSpan.Zero)
+                            {
+                                cacheControl = new CacheControlHeaderValue
+                                {
+                                    MinFresh = httpRequestCachePolicy.MinFresh
+                                };
+                            }
+
+                            if (httpRequestCachePolicy.MaxAge != TimeSpan.MaxValue)
+                            {
+                                cacheControl = new CacheControlHeaderValue
+                                {
+                                    MaxAge = httpRequestCachePolicy.MaxAge
+                                };
+                            }
+
+                            if (httpRequestCachePolicy.MaxStale > TimeSpan.Zero)
+                            {
+                                cacheControl = new CacheControlHeaderValue
+                                {
+                                    MaxStale = true,
+                                    MaxStaleLimit = httpRequestCachePolicy.MaxStale
+                                };
+                            }


MaxAge and MaxStale will both override the previous settings here. Seems like there should only be one new CacheControlHeaderValue.

scalablecory · 2021-11-08T17:49:38Z

src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs

+            if (policy != null && policy.Level != RequestCacheLevel.BypassCache)
+            {
+                CacheControlHeaderValue? cacheControl = null;
+                var pragmaHeaders = request.Headers.Pragma;


We only var if the right hand side is new T. This makes sure the code is easy to read outside of an IDE.

scalablecory · 2021-11-08T18:01:42Z

src/libraries/System.Net.Requests/src/System/Net/HttpWebRequest.cs

+            {
+                return DefaultCachePolicy;
+            }
+            else if (WebRequest.DefaultCachePolicy != null)


Suggested change

else if (WebRequest.DefaultCachePolicy != null)

else

scalablecory

looks good, thanks for the PR!

Request CachePolicy isn't applied in HTTP request header

59b5a15

dotnet-issue-labeler bot added the area-System.Net label Oct 27, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 27, 2021

pedrobsaila mentioned this pull request Oct 27, 2021

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

Closed

handle other CachePolicy levels

3d4bf86

manage max-stale cache policy case

e272a65

handle DefaultCachePolicy

9ed19f3

karelz requested a review from scalablecory November 2, 2021 20:21

karelz assigned scalablecory Nov 2, 2021

CachePolicy is nullable by default

ac36aec

scalablecory suggested changes Nov 3, 2021

View reviewed changes

explicitely set DefaultCachePolicy and DefaultCachePolicy tests execu…

32dffd6

…tion on side process

scalablecory suggested changes Nov 8, 2021

View reviewed changes

fix minor issues

f499323

scalablecory approved these changes Nov 9, 2021

View reviewed changes

scalablecory merged commit 26c045b into dotnet:main Nov 9, 2021

pedrobsaila deleted the 31320 branch November 9, 2021 19:44

karelz added this to the 7.0.0 milestone Nov 23, 2021

This was referenced Nov 30, 2021

bot upstream main merge 2021 11 30 Unity-Technologies/runtime#10

Closed

bot upstream main merge 2021 11 30 Unity-Technologies/runtime#11

Closed

bot upstream main merge 2021 12 01 Unity-Technologies/runtime#12

Closed

ghost locked as resolved and limited conversation to collaborators Dec 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pedrobsaila commented Oct 27, 2021

ghost commented Oct 27, 2021

pedrobsaila commented Oct 27, 2021 •

edited

Loading

scalablecory commented Nov 1, 2021

pedrobsaila commented Nov 2, 2021 •

edited

Loading

scalablecory commented Nov 2, 2021

scalablecory left a comment

scalablecory Nov 3, 2021

scalablecory Nov 3, 2021

scalablecory Nov 3, 2021 •

edited

Loading

pedrobsaila Nov 4, 2021 •

edited

Loading

scalablecory Nov 4, 2021

scalablecory Nov 3, 2021

pedrobsaila commented Nov 4, 2021 •

edited

Loading

scalablecory commented Nov 4, 2021

scalablecory left a comment

scalablecory Nov 8, 2021

scalablecory Nov 8, 2021

scalablecory Nov 8, 2021

scalablecory left a comment

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

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

Conversation

pedrobsaila commented Oct 27, 2021

ghost commented Oct 27, 2021

pedrobsaila commented Oct 27, 2021 • edited Loading

scalablecory commented Nov 1, 2021

pedrobsaila commented Nov 2, 2021 • edited Loading

scalablecory commented Nov 2, 2021

scalablecory left a comment

Choose a reason for hiding this comment

scalablecory Nov 3, 2021

Choose a reason for hiding this comment

scalablecory Nov 3, 2021

Choose a reason for hiding this comment

scalablecory Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

pedrobsaila Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

scalablecory Nov 4, 2021

Choose a reason for hiding this comment

scalablecory Nov 3, 2021

Choose a reason for hiding this comment

pedrobsaila commented Nov 4, 2021 • edited Loading

scalablecory commented Nov 4, 2021

scalablecory left a comment

Choose a reason for hiding this comment

scalablecory Nov 8, 2021

Choose a reason for hiding this comment

scalablecory Nov 8, 2021

Choose a reason for hiding this comment

scalablecory Nov 8, 2021

Choose a reason for hiding this comment

scalablecory left a comment

Choose a reason for hiding this comment

pedrobsaila commented Oct 27, 2021 •

edited

Loading

pedrobsaila commented Nov 2, 2021 •

edited

Loading

scalablecory Nov 3, 2021 •

edited

Loading

pedrobsaila Nov 4, 2021 •

edited

Loading

pedrobsaila commented Nov 4, 2021 •

edited

Loading