Issue 455623003: stale-while-revalidate experimental implementation.

Adam Rice

6 years, 4 months ago (2014-08-25 15:26:59 UTC) #1

yhirano

https://codereview.chromium.org/455623003/diff/140001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/455623003/diff/140001/chrome/browser/io_thread.cc#newcode712 chrome/browser/io_thread.cc:712: if (base::FieldTrialList::FindFullName(kStaleWhileRevalidateFieldTrialName) == Can you tell me when and ...

6 years, 3 months ago (2014-08-26 07:52:02 UTC) #2

Adam Rice

https://codereview.chromium.org/455623003/diff/140001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/455623003/diff/140001/chrome/browser/io_thread.cc#newcode712 chrome/browser/io_thread.cc:712: if (base::FieldTrialList::FindFullName(kStaleWhileRevalidateFieldTrialName) == On 2014/08/26 07:52:01, yhirano wrote: > ...

6 years, 3 months ago (2014-08-26 13:38:43 UTC) #3

Adam Rice

Sorry about the extra changes. I have stopped adding stuff now. PTAL.

6 years, 3 months ago (2014-08-27 04:36:50 UTC) #4

yhirano

https://codereview.chromium.org/455623003/diff/240001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/240001/net/http/http_cache.cc#newcode335 net/http/http_cache.cc:335: if (network_delegate) { Can you pass before_proxy_headers_sent_callback_ from HttpCache::Transaction? ...

6 years, 3 months ago (2014-08-27 09:24:29 UTC) #5

Adam Rice

I will have to add another patch set after this one because I found a ...

6 years, 3 months ago (2014-08-28 03:14:35 UTC) #6

Adam Rice

ricea@chromium.org changed reviewers: + rvargas@chromium.org

6 years, 3 months ago (2014-08-28 11:42:17 UTC) #8

Adam Rice

+rvargas PTAL. As you probably know, there has been some controversy about checking this in ...

6 years, 3 months ago (2014-08-28 11:42:17 UTC) #9

rvargas (doing something else)

I'm sorry we didn't catch issues with the design doc, especially because it is exceptionally ...

6 years, 3 months ago (2014-08-29 23:36:07 UTC) #10

I'm sorry we didn't catch issues with the design doc, especially because it is
exceptionally detailed. I guess it's just easier to see how things work with a
real code review :(

My concern at this point is that I don't think long term we'll be able to keep
the async revalidation code in the HTTP cache. To be honest, I actually thought
that when reading the (first?) version of the doc. In part because it lacks
context to know when to issue the request, and in part because all the logic
handled at higher layers. I should have been more explicit.

So I'm a little weary of adding code at the wrong layer, even if that is the
first rough implementation. We would have to remove almost all of this code when
going to the "right" place.

I'm also concerned about the results that we would get. There's a lot of caveats
and I don't have a good sense of what would we say if the results are not good.
That they can improve with the second version?

On the other hand, I'm open to accept the phase 2 approach for Dev channel to
start the ball rolling while at the same time we start implementing phase 3 for
Beta channels.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:334: transaction_.reset(new Transaction(IDLE, cache_));
It's better to keep transaction creation centralized in CreateTransaction

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:359: transaction_->ValidateInternalCache();
This looks out of the blue

I just found the mention of it one the design doc. I think things should go
through standard interfaces, and that means load flags. The code on SetRequest
is very surprising because of this line.

(forcing upstream caches to revalidate sounds ok)

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(),
transaction_.get());
why? Looks like a problem with this code, not with the entry.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:414:
UMA_HISTOGRAM_TIMES("HttpCache.AsyncValidationTimeSaved", time_saved);
What is this measuring? This cannot be accounted as the PLT time that was saved
by using this feature, right?

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:1180: DCHECK(insert_ok);
why can't this fail?

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:1185: job->Start(net_log, network_delegate);
AsyncValidation is not a UrlRequest::Job so it's better to use another name
here.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:57: const ValidationType VALIDATION_NONE =
ValidationType::VALIDATION_NONE;
I think we should just live with HttpResponseHeaders::VALIDATION_NONE instead.
If that is too ugly, move the enum out of the class.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2328: response_.headers->RequiresValidation(
If you keep the name for ResponseHeaders::RequiresValidation (which is fine),
you should also keep the name for Transaction::RequiresValidation (conceptually
they do the same thing)

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2338: if (request_->method == "GET" &&
(truncated_ || partial_))
This part belong on the caller side.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2394:
response_.headers->GetFreshnessLifetime(response_.response_time,
This pattern reads very weird: returning a value that looks time related and the
actual time through an out argument, especially considering the name of the
method.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2589: cache_,
Is the intention here to use a weak pointer? (I bet it is).

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
net/http/http_response_headers.cc:964: HttpResponseHeaders::ValidationType
HttpResponseHeaders::RequiresValidation(
We don't need the class name on the return value, do we?

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
net/http/http_response_headers.cc:1004: HttpResponseHeaders::FreshnessType
HttpResponseHeaders::GetFreshnessLifetime(
:(

Can we just say that in order to have async revalidations the lifetime has to be
grater than zero? If a server is changing to add the new header, it might as
well set max_age or similar. Use revalidation with lifetime of zero is a
confusing signal to me: "always check with the server, and by the way, use it
for a while without checking with the server"

Adam Rice

I have added a section to the implementation design doc about how we back out ...

6 years, 3 months ago (2014-09-01 14:43:00 UTC) #11

I have added a section to the implementation design doc about how we back out
these changes after the experiment. Because of the narrow scope, they should be
relatively easy to revert.

I also added a section on success/failure criteria for the experiment. This is
probably out-of-place in an implementation design doc, but I feel because of the
deficiencies in this experimental implementation it is important to state
up-front what the expectations are. 

I don't want to put a lot of work into the "real" implementation without some
evidence of concrete benefit.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:334: transaction_.reset(new Transaction(IDLE, cache_));
On 2014/08/29 23:36:06, rvargas wrote:
> It's better to keep transaction creation centralized in CreateTransaction

Done.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:359: transaction_->ValidateInternalCache();
On 2014/08/29 23:36:06, rvargas wrote:
> This looks out of the blue
> 
> I just found the mention of it one the design doc. I think things should go
> through standard interfaces, and that means load flags. The code on SetRequest
> is very surprising because of this line.
> 
> (forcing upstream caches to revalidate sounds ok)

I originally was using load flags, but it made me uncomfortable that it set
Cache-Control: max-age=0. The more I've thought about it, the more I've become
convinced that the asynchronous request should be the same as the synchronous
request it replaces.

In particular, servers behind reverse proxies may incur considerable extra cost
when max-age=0 is set on the request, to the point where it may be uneconomical
for them to use stale-while-revalidate at all.

That's not to say that I think this is a wonderful API. In the future I envision
using something like a LOAD_ASYNC_REVALIDATION flag to make sure we don't
re-enter the async pathway. But I already knew I was going to throw away this
implementation when I added this.

Other than adding a new load flag, I can't see a way to do this with existing
interfaces without side-effects.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(),
transaction_.get());
On 2014/08/29 23:36:06, rvargas wrote:
> why? Looks like a problem with this code, not with the entry.

Consider the case where the remote server suddenly asks for a client
certificate. With this design, there's no way we can satisfy the request.

However, even in a different design where SSL certificate errors can be handled,
we shouldn't pop-up UI for an asynchronous request. So if we need to ask the
user to select a client certificate, we can't do it now, and the request should
fail.

The ideal behaviour is to make sure that if the user hits the same resource
again, the next time we make a synchronous request.

This could maybe be done by adding a flag to HttpResponseInfo, but for the sake
of simplicity this initial implementation just dooms the entry.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:414:
UMA_HISTOGRAM_TIMES("HttpCache.AsyncValidationTimeSaved", time_saved);
On 2014/08/29 23:36:06, rvargas wrote:
> What is this measuring? This cannot be accounted as the PLT time that was
saved
> by using this feature, right?

Sadly, no.

I was thinking of splitting it into two histograms: CSS/sync JS/fonts--"things
that are in the critical path to display the page" and "other". But it appears
we can't distinguish sync JS from async JS at this layer. Also, even things in
the critical path can be loaded in parallel, so it wouldn't be all that
meaningful.

It's more of an "AsyncValidationDidSomething" metric than anything else. I can't
think of a better name for it right now.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:1180: DCHECK(insert_ok);
On 2014/08/29 23:36:06, rvargas wrote:
> why can't this fail?

It is matched with the erase() in DeleteAsyncValidation. new AsyncValidation()
cannot return the same pointer until the old one is freed, and AsyncValidations
are only freed in DeleteAsyncValidation().

Strictly speaking, they are also freed in the ~HttpCache() destructor, but that
calls InvalidateWeakPtrs() first, ensuring that PerformAsyncValidation() cannot
be called.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:1185: job->Start(net_log, network_delegate);
On 2014/08/29 23:36:06, rvargas wrote:
> AsyncValidation is not a UrlRequest::Job so it's better to use another name
> here.

Done.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:57: const ValidationType VALIDATION_NONE =
ValidationType::VALIDATION_NONE;
On 2014/08/29 23:36:07, rvargas wrote:
> I think we should just live with HttpResponseHeaders::VALIDATION_NONE instead.
> If that is too ugly, move the enum out of the class.

Done.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2328: response_.headers->RequiresValidation(
On 2014/08/29 23:36:06, rvargas wrote:
> If you keep the name for ResponseHeaders::RequiresValidation (which is fine),
> you should also keep the name for Transaction::RequiresValidation
(conceptually
> they do the same thing)

Done.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2338: if (request_->method == "GET" &&
(truncated_ || partial_))
On 2014/08/29 23:36:06, rvargas wrote:
> This part belong on the caller side.

Done.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2394:
response_.headers->GetFreshnessLifetime(response_.response_time,
On 2014/08/29 23:36:07, rvargas wrote:
> This pattern reads very weird: returning a value that looks time related and
the
> actual time through an out argument, especially considering the name of the
> method.

I have attempted to improve it. PTAL.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2589: cache_,
On 2014/08/29 23:36:06, rvargas wrote:
> Is the intention here to use a weak pointer? (I bet it is).

Yes. I have added a comment.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
net/http/http_response_headers.cc:964: HttpResponseHeaders::ValidationType
HttpResponseHeaders::RequiresValidation(
On 2014/08/29 23:36:07, rvargas wrote:
> We don't need the class name on the return value, do we?

Yes, we do.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
net/http/http_response_headers.cc:1004: HttpResponseHeaders::FreshnessType
HttpResponseHeaders::GetFreshnessLifetime(
On 2014/08/29 23:36:07, rvargas wrote:
> :(
> 
> Can we just say that in order to have async revalidations the lifetime has to
be
> grater than zero? If a server is changing to add the new header, it might as
> well set max_age or similar. Use revalidation with lifetime of zero is a
> confusing signal to me: "always check with the server, and by the way, use it
> for a while without checking with the server"

The problem is that max-age=0,stale-while-revalidate=300 does have a
well-defined behaviour, and is indeed in use.
http://www.myspass.de/myspass/media/images/pix.gif and a number of other URLs
from that domain are served with:

Cache-Control:
must-revalidate,max-age=0,s-maxage=300,stale-while-revalidate=300,stale-if-error=420,
must-revalidate,max-age=0,s-maxage=300,stale-while-revalidate=300,stale-if-error=420

I can try to make the implementation less ugly, but I don't think we should
ignore this case just because it makes the code ugly.

rvargas (doing something else)

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#newcode359 net/http/http_cache.cc:359: transaction_->ValidateInternalCache(); On 2014/09/01 14:43:00, Adam Rice wrote: > On ...

6 years, 3 months ago (2014-09-03 01:25:53 UTC) #12

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:359: transaction_->ValidateInternalCache();
On 2014/09/01 14:43:00, Adam Rice wrote:
> On 2014/08/29 23:36:06, rvargas wrote:
> > This looks out of the blue
> > 
> > I just found the mention of it one the design doc. I think things should go
> > through standard interfaces, and that means load flags. The code on
SetRequest
> > is very surprising because of this line.
> > 
> > (forcing upstream caches to revalidate sounds ok)
> 
> I originally was using load flags, but it made me uncomfortable that it set
> Cache-Control: max-age=0. The more I've thought about it, the more I've become
> convinced that the asynchronous request should be the same as the synchronous
> request it replaces.
> 
> In particular, servers behind reverse proxies may incur considerable extra
cost
> when max-age=0 is set on the request, to the point where it may be
uneconomical
> for them to use stale-while-revalidate at all.
> 
> That's not to say that I think this is a wonderful API. In the future I
envision
> using something like a LOAD_ASYNC_REVALIDATION flag to make sure we don't
> re-enter the async pathway. But I already knew I was going to throw away this
> implementation when I added this.
> 
> Other than adding a new load flag, I can't see a way to do this with existing
> interfaces without side-effects.

I think it is interesting to bring this up. Basically you are saying that an
user hitting ctrl-f5 causes a very big burden on intermediary proxies to the
point that it changes the economics of the lifetime for a resource. When a user
hits ctrl-F5, we say that whatever we are showing may be out of date and we
force all upstream proxies to revalidate their content with the server. In this
case we are also saying that we believe we may have outdated resources and we
want to double check them with the origin server. It sounds very similar, but I
see your point. Assuming _all_ proxies in the middle understand SWR, an extra
header should not be needed.

From an implementation point of view, I'd prefer to define an extra load flag
now and not have a sidechannel to the cache transaction.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(),
transaction_.get());
On 2014/09/01 14:43:00, Adam Rice wrote:
> On 2014/08/29 23:36:06, rvargas wrote:
> > why? Looks like a problem with this code, not with the entry.
> 
> Consider the case where the remote server suddenly asks for a client
> certificate. With this design, there's no way we can satisfy the request.
> 
> However, even in a different design where SSL certificate errors can be
handled,
> we shouldn't pop-up UI for an asynchronous request. So if we need to ask the
> user to select a client certificate, we can't do it now, and the request
should
> fail.
> 
> The ideal behaviour is to make sure that if the user hits the same resource
> again, the next time we make a synchronous request.
> 
> This could maybe be done by adding a flag to HttpResponseInfo, but for the
sake
> of simplicity this initial implementation just dooms the entry.

We don't have to pop up an auth dialog for every resource that is loaded from a
given domain. The auth layer deals with that. I'm quite wary of deleting entries
because we don't have the right code in place... so either the code uses other
code at the 'right' layer or it replicates the required logic.

Even on the final design, if an auth dialog is to be triggered with an async
request because no resource was required from the server before, I would not be
very fond of deleting entries from the cache... even if that means that the
resource is not validated against the server and we have to wait until some auth
credentials are in place.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:414:
UMA_HISTOGRAM_TIMES("HttpCache.AsyncValidationTimeSaved", time_saved);
On 2014/09/01 14:43:00, Adam Rice wrote:
> On 2014/08/29 23:36:06, rvargas wrote:
> > What is this measuring? This cannot be accounted as the PLT time that was
> saved
> > by using this feature, right?
> 
> Sadly, no.
> 
> I was thinking of splitting it into two histograms: CSS/sync JS/fonts--"things
> that are in the critical path to display the page" and "other". But it appears
> we can't distinguish sync JS from async JS at this layer. Also, even things in
> the critical path can be loaded in parallel, so it wouldn't be all that
> meaningful.
> 
> It's more of an "AsyncValidationDidSomething" metric than anything else. I
can't
> think of a better name for it right now.

I'm fine with it as long as we don't use as a metric for the success of the
experiment.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:1180: DCHECK(insert_ok);
On 2014/09/01 14:43:00, Adam Rice wrote:
> On 2014/08/29 23:36:06, rvargas wrote:
> > why can't this fail?
> 
> It is matched with the erase() in DeleteAsyncValidation. new AsyncValidation()
> cannot return the same pointer until the old one is freed, and
AsyncValidations
> are only freed in DeleteAsyncValidation().
> 
> Strictly speaking, they are also freed in the ~HttpCache() destructor, but
that
> calls InvalidateWeakPtrs() first, ensuring that PerformAsyncValidation()
cannot
> be called.

But we should not be performing two async validations for the same resource at
the same time, right? Shouldn't we ignore a second request if a another is in
progress?

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
net/http/http_response_headers.cc:1004: HttpResponseHeaders::FreshnessType
HttpResponseHeaders::GetFreshnessLifetime(
On 2014/09/01 14:43:00, Adam Rice wrote:
> On 2014/08/29 23:36:07, rvargas wrote:
> > :(
> > 
> > Can we just say that in order to have async revalidations the lifetime has
to
> be
> > grater than zero? If a server is changing to add the new header, it might as
> > well set max_age or similar. Use revalidation with lifetime of zero is a
> > confusing signal to me: "always check with the server, and by the way, use
it
> > for a while without checking with the server"
> 
> The problem is that max-age=0,stale-while-revalidate=300 does have a
> well-defined behaviour, and is indeed in use.
> http://www.myspass.de/myspass/media/images/pix.gif and a number of other URLs
> from that domain are served with:
> 
> Cache-Control:
>
must-revalidate,max-age=0,s-maxage=300,stale-while-revalidate=300,stale-if-error=420,
>
must-revalidate,max-age=0,s-maxage=300,stale-while-revalidate=300,stale-if-error=420
> 
> I can try to make the implementation less ugly, but I don't think we should
> ignore this case just because it makes the code ugly.

The ugly code (two, maybe contradictory outputs) is just the consequence of the
conceptual contradiction I mentioned, which is what I don't like.

Note that the spec says:
  "Note that "stale" implies that the response will have a non-zero Age
   header and a warning header, as per HTTP's requirements."

So I read that as saying that the headers from that site are invalid, or at
least that we can ignore the SWR part.

Adam Rice

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#newcode359 net/http/http_cache.cc:359: transaction_->ValidateInternalCache(); On 2014/09/03 01:25:53, rvargas wrote: > On 2014/09/01 ...

6 years, 3 months ago (2014-09-05 15:17:11 UTC) #13

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:359: transaction_->ValidateInternalCache();
On 2014/09/03 01:25:53, rvargas wrote:
> On 2014/09/01 14:43:00, Adam Rice wrote:
> > On 2014/08/29 23:36:06, rvargas wrote:
> > > This looks out of the blue
> > > 
> > > I just found the mention of it one the design doc. I think things should
go
> > > through standard interfaces, and that means load flags. The code on
> SetRequest
> > > is very surprising because of this line.
> > > 
> > > (forcing upstream caches to revalidate sounds ok)
> > 
> > I originally was using load flags, but it made me uncomfortable that it set
> > Cache-Control: max-age=0. The more I've thought about it, the more I've
become
> > convinced that the asynchronous request should be the same as the
synchronous
> > request it replaces.
> > 
> > In particular, servers behind reverse proxies may incur considerable extra
> cost
> > when max-age=0 is set on the request, to the point where it may be
> uneconomical
> > for them to use stale-while-revalidate at all.
> > 
> > That's not to say that I think this is a wonderful API. In the future I
> envision
> > using something like a LOAD_ASYNC_REVALIDATION flag to make sure we don't
> > re-enter the async pathway. But I already knew I was going to throw away
this
> > implementation when I added this.
> > 
> > Other than adding a new load flag, I can't see a way to do this with
existing
> > interfaces without side-effects.
> 
> I think it is interesting to bring this up. Basically you are saying that an
> user hitting ctrl-f5 causes a very big burden on intermediary proxies to the
> point that it changes the economics of the lifetime for a resource. When a
user
> hits ctrl-F5, we say that whatever we are showing may be out of date and we
> force all upstream proxies to revalidate their content with the server. In
this
> case we are also saying that we believe we may have outdated resources and we
> want to double check them with the origin server. It sounds very similar, but
I
> see your point. Assuming _all_ proxies in the middle understand SWR, an extra
> header should not be needed.
> 
> From an implementation point of view, I'd prefer to define an extra load flag
> now and not have a sidechannel to the cache transaction.

Okay, I have added LOAD_ASYNC_REVALIDATION and removed the extra API.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(),
transaction_.get());
On 2014/09/03 01:25:53, rvargas wrote:
> On 2014/09/01 14:43:00, Adam Rice wrote:
> > On 2014/08/29 23:36:06, rvargas wrote:
> > > why? Looks like a problem with this code, not with the entry.
> > 
> > Consider the case where the remote server suddenly asks for a client
> > certificate. With this design, there's no way we can satisfy the request.
> > 
> > However, even in a different design where SSL certificate errors can be
> handled,
> > we shouldn't pop-up UI for an asynchronous request. So if we need to ask the
> > user to select a client certificate, we can't do it now, and the request
> should
> > fail.
> > 
> > The ideal behaviour is to make sure that if the user hits the same resource
> > again, the next time we make a synchronous request.
> > 
> > This could maybe be done by adding a flag to HttpResponseInfo, but for the
> sake
> > of simplicity this initial implementation just dooms the entry.
> 
> We don't have to pop up an auth dialog for every resource that is loaded from
a
> given domain. The auth layer deals with that. I'm quite wary of deleting
entries
> because we don't have the right code in place... so either the code uses other
> code at the 'right' layer or it replicates the required logic.
> 
> Even on the final design, if an auth dialog is to be triggered with an async
> request because no resource was required from the server before, I would not
be
> very fond of deleting entries from the cache... even if that means that the
> resource is not validated against the server and we have to wait until some
auth
> credentials are in place.

For the final design, I'm thinking of something like having an
async_revalidation_failed flag in HttpResponseInfo that would prevent entering
the async path for the same resource again. But that would mean a disk cache
format change, so it's not something I'd do lightly.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:414:
UMA_HISTOGRAM_TIMES("HttpCache.AsyncValidationTimeSaved", time_saved);
On 2014/09/03 01:25:53, rvargas wrote:
> On 2014/09/01 14:43:00, Adam Rice wrote:
> > On 2014/08/29 23:36:06, rvargas wrote:
> > > What is this measuring? This cannot be accounted as the PLT time that was
> > saved
> > > by using this feature, right?
> > 
> > Sadly, no.
> > 
> > I was thinking of splitting it into two histograms: CSS/sync
JS/fonts--"things
> > that are in the critical path to display the page" and "other". But it
appears
> > we can't distinguish sync JS from async JS at this layer. Also, even things
in
> > the critical path can be loaded in parallel, so it wouldn't be all that
> > meaningful.
> > 
> > It's more of an "AsyncValidationDidSomething" metric than anything else. I
> can't
> > think of a better name for it right now.
> 
> I'm fine with it as long as we don't use as a metric for the success of the
> experiment.

I renamed it AsyncValidationDuration to avoid misunderstandings.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:1180: DCHECK(insert_ok);
On 2014/09/03 01:25:52, rvargas wrote:
> On 2014/09/01 14:43:00, Adam Rice wrote:
> > On 2014/08/29 23:36:06, rvargas wrote:
> > > why can't this fail?
> > 
> > It is matched with the erase() in DeleteAsyncValidation. new
AsyncValidation()
> > cannot return the same pointer until the old one is freed, and
> AsyncValidations
> > are only freed in DeleteAsyncValidation().
> > 
> > Strictly speaking, they are also freed in the ~HttpCache() destructor, but
> that
> > calls InvalidateWeakPtrs() first, ensuring that PerformAsyncValidation()
> cannot
> > be called.
> 
> But we should not be performing two async validations for the same resource at
> the same time, right? Shouldn't we ignore a second request if a another is in
> progress?

It might be possible to get multiple AsyncValidations to the same URL due to a
race condition, but I haven't managed to get it to happen in my tests. All that
will happen is that they will all wait behind the cache lock for the first one
to complete, and then (assuming the revalidation was successful) complete from
the cache.

Anyway, the std::set is only comparing the pointers, it doesn't care whether
they refer to the same resource or not.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
net/http/http_response_headers.cc:1004: HttpResponseHeaders::FreshnessType
HttpResponseHeaders::GetFreshnessLifetime(
On 2014/09/03 01:25:53, rvargas wrote:
> On 2014/09/01 14:43:00, Adam Rice wrote:
> > On 2014/08/29 23:36:07, rvargas wrote:
> > > :(
> > > 
> > > Can we just say that in order to have async revalidations the lifetime has
> to
> > be
> > > grater than zero? If a server is changing to add the new header, it might
as
> > > well set max_age or similar. Use revalidation with lifetime of zero is a
> > > confusing signal to me: "always check with the server, and by the way, use
> it
> > > for a while without checking with the server"
> > 
> > The problem is that max-age=0,stale-while-revalidate=300 does have a
> > well-defined behaviour, and is indeed in use.
> > http://www.myspass.de/myspass/media/images/pix.gif and a number of other
URLs
> > from that domain are served with:
> > 
> > Cache-Control:
> >
>
must-revalidate,max-age=0,s-maxage=300,stale-while-revalidate=300,stale-if-error=420,
> >
>
must-revalidate,max-age=0,s-maxage=300,stale-while-revalidate=300,stale-if-error=420
> > 
> > I can try to make the implementation less ugly, but I don't think we should
> > ignore this case just because it makes the code ugly.
> 
> The ugly code (two, maybe contradictory outputs) is just the consequence of
the
> conceptual contradiction I mentioned, which is what I don't like.
> 
> Note that the spec says:
>   "Note that "stale" implies that the response will have a non-zero Age
>    header and a warning header, as per HTTP's requirements."
> 
> So I read that as saying that the headers from that site are invalid, or at
> least that we can ignore the SWR part.

If this is true, then it should apply also for max-age=0 by itself, regardless
of any other Cache-Control directives.

There are 4 sites in the HTTP Archive that set max-age=0 along with a Warning:
110 and an Age: header. And 53,000 that don't.

I think people are voting with their feet that max-age=0 does not mean stale.
Although it definitely doesn't mean fresh either.

I personally don't see a contradiction. The way I see it, max-age=0 is slightly
different from Cache-Control: no-cache and distinctly different from
Cache-Control: no-store, and Chrome's cache has only gotten away with treating
them the same up until now because it never supported stale resources.

Once we support stale resources then the differences start to matter and we
can't treat them the same any more.

For what it's worth, I've changed the "Freshness" type slightly so that it is
impossible to set contradictory values (at least not without doing ugly tricky
with const_cast).

rvargas (doing something else)

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#newcode412 net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(), transaction_.get()); On 2014/09/05 15:17:11, Adam Rice wrote: > ...

6 years, 3 months ago (2014-09-05 22:52:40 UTC) #14

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(),
transaction_.get());
On 2014/09/05 15:17:11, Adam Rice wrote:
> On 2014/09/03 01:25:53, rvargas wrote:
> > On 2014/09/01 14:43:00, Adam Rice wrote:
> > > On 2014/08/29 23:36:06, rvargas wrote:
> > > > why? Looks like a problem with this code, not with the entry.
> > > 
> > > Consider the case where the remote server suddenly asks for a client
> > > certificate. With this design, there's no way we can satisfy the request.
> > > 
> > > However, even in a different design where SSL certificate errors can be
> > handled,
> > > we shouldn't pop-up UI for an asynchronous request. So if we need to ask
the
> > > user to select a client certificate, we can't do it now, and the request
> > should
> > > fail.
> > > 
> > > The ideal behaviour is to make sure that if the user hits the same
resource
> > > again, the next time we make a synchronous request.
> > > 
> > > This could maybe be done by adding a flag to HttpResponseInfo, but for the
> > sake
> > > of simplicity this initial implementation just dooms the entry.
> > 
> > We don't have to pop up an auth dialog for every resource that is loaded
from
> a
> > given domain. The auth layer deals with that. I'm quite wary of deleting
> entries
> > because we don't have the right code in place... so either the code uses
other
> > code at the 'right' layer or it replicates the required logic.
> > 
> > Even on the final design, if an auth dialog is to be triggered with an async
> > request because no resource was required from the server before, I would not
> be
> > very fond of deleting entries from the cache... even if that means that the
> > resource is not validated against the server and we have to wait until some
> auth
> > credentials are in place.
> 
> For the final design, I'm thinking of something like having an
> async_revalidation_failed flag in HttpResponseInfo that would prevent entering
> the async path for the same resource again. But that would mean a disk cache
> format change, so it's not something I'd do lightly.

My suggestion for the final design would be that the revalidation would not be
initiated by the cache, so whoever takes care of driving the revalidation should
decide what to do.

As for this version, I'm still not convinced of deleting the entry. At least not
without first trying to solve the problem.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:1180: DCHECK(insert_ok);
On 2014/09/05 15:17:11, Adam Rice wrote:
> On 2014/09/03 01:25:52, rvargas wrote:
> > On 2014/09/01 14:43:00, Adam Rice wrote:
> > > On 2014/08/29 23:36:06, rvargas wrote:
> > > > why can't this fail?
> > > 
> > > It is matched with the erase() in DeleteAsyncValidation. new
> AsyncValidation()
> > > cannot return the same pointer until the old one is freed, and
> > AsyncValidations
> > > are only freed in DeleteAsyncValidation().
> > > 
> > > Strictly speaking, they are also freed in the ~HttpCache() destructor, but
> > that
> > > calls InvalidateWeakPtrs() first, ensuring that PerformAsyncValidation()
> > cannot
> > > be called.
> > 
> > But we should not be performing two async validations for the same resource
at
> > the same time, right? Shouldn't we ignore a second request if a another is
in
> > progress?
> 
> It might be possible to get multiple AsyncValidations to the same URL due to a
> race condition, but I haven't managed to get it to happen in my tests. All
that
> will happen is that they will all wait behind the cache lock for the first one
> to complete, and then (assuming the revalidation was successful) complete from
> the cache.
> 
> Anyway, the std::set is only comparing the pointers, it doesn't care whether
> they refer to the same resource or not.

Sorry I was not clear. Yes, things will be queued by the lock (so there is no
race), but that means consecutive revalidations are going through the wire.
Clearly, the first revalidation should invalidate the others as the assumption
that a revalidation is needed doesn't hold anymore.

So while I see why a DCHECK here (with the current implementation) should never
fire (and it is a good invariant check), I think we should be keeping track of
the URL being revalidated and not queue another revalidation for the same
resource.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_response_...
net/http/http_response_headers.cc:1004: HttpResponseHeaders::FreshnessType
HttpResponseHeaders::GetFreshnessLifetime(
On 2014/09/05 15:17:11, Adam Rice wrote:
> On 2014/09/03 01:25:53, rvargas wrote:
> > On 2014/09/01 14:43:00, Adam Rice wrote:
> > > On 2014/08/29 23:36:07, rvargas wrote:
> > > > :(
> > > > 
> > > > Can we just say that in order to have async revalidations the lifetime
has
> > to
> > > be
> > > > grater than zero? If a server is changing to add the new header, it
might
> as
> > > > well set max_age or similar. Use revalidation with lifetime of zero is a
> > > > confusing signal to me: "always check with the server, and by the way,
use
> > it
> > > > for a while without checking with the server"
> > > 
> > > The problem is that max-age=0,stale-while-revalidate=300 does have a
> > > well-defined behaviour, and is indeed in use.
> > > http://www.myspass.de/myspass/media/images/pix.gif and a number of other
> URLs
> > > from that domain are served with:
> > > 
> > > Cache-Control:
> > >
> >
>
must-revalidate,max-age=0,s-maxage=300,stale-while-revalidate=300,stale-if-error=420,
> > >
> >
>
must-revalidate,max-age=0,s-maxage=300,stale-while-revalidate=300,stale-if-error=420
> > > 
> > > I can try to make the implementation less ugly, but I don't think we
should
> > > ignore this case just because it makes the code ugly.
> > 
> > The ugly code (two, maybe contradictory outputs) is just the consequence of
> the
> > conceptual contradiction I mentioned, which is what I don't like.
> > 
> > Note that the spec says:
> >   "Note that "stale" implies that the response will have a non-zero Age
> >    header and a warning header, as per HTTP's requirements."
> > 
> > So I read that as saying that the headers from that site are invalid, or at
> > least that we can ignore the SWR part.
> 
> If this is true, then it should apply also for max-age=0 by itself, regardless
> of any other Cache-Control directives.

I don't follow what you mean.

> 
> There are 4 sites in the HTTP Archive that set max-age=0 along with a Warning:
> 110 and an Age: header. And 53,000 that don't.

Same here.

> 
> I think people are voting with their feet that max-age=0 does not mean stale.
> Although it definitely doesn't mean fresh either.
> 
> I personally don't see a contradiction. The way I see it, max-age=0 is
slightly
> different from Cache-Control: no-cache and distinctly different from
> Cache-Control: no-store, and Chrome's cache has only gotten away with treating
> them the same up until now because it never supported stale resources.

Well... I guess I know where you're going. The Warning header is meant to be
used by proxies to tell clients that they are returning resources that may be
stale. I don't think we will ever show a dialog to the user saying that
something may be stale, in the context of stale as understood by the HTTP spec.
(nor in the context of the SWR RFC). So the appearance in the quote that I made
was circumstantial.

But you are right, max-age and no-cache are different if we return stale entries
:(

(see the other comments)

> 
> Once we support stale resources then the differences start to matter and we
> can't treat them the same any more.
> 
> For what it's worth, I've changed the "Freshness" type slightly so that it is
> impossible to set contradictory values (at least not without doing ugly tricky
> with const_cast).

https://codereview.chromium.org/455623003/diff/320001/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/455623003/diff/320001/net/base/load_flags_lis...
net/base/load_flags_list.h:130: // flag just prevents re-entering the async code
path, so you probably don't
nit: The comment risks becoming stale without noticing. When we move issuing the
request above the cache there has to be a way to tell the cache not to serve
stale content again, right?

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.cc#...
net/http/http_cache.cc:360:
transaction_.reset(static_cast<Transaction*>(transaction.release()));
... or, pass the transaction at construction...

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.h#n...
net/http/http_cache.h:454: // live AsyncValidation objects connected to this
cache are in this set.
Isn't the invariant the expected behavior? Why document something that would be
extremely weird to violate?

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2114: bool skip_validation =
required_validation == ValidationType::VALIDATION_NONE;
nit: use () around ==

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2308: return ValidationType::VALIDATION_NONE;
I think we should move the enum out of ResponseHeaders. It clearly has to be
used by other parts of the code when not interacting directly with response
headers and it feels weird that load flags can be used as they are and
validation type cannot. Besides, it is redundant to effectively have
'validation' twice in the name of each value.

(just to be clear, defining the enum in http_response_headers.h is fine)

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2310: if (effective_load_flags_ &
(LOAD_VALIDATE_CACHE | LOAD_ASYNC_REVALIDATION))
I just realized, can't we use LOAD_VALIDATE_CACHE instead of defining the new
flag? This one doesn't add headers to blast proxies, and it also makes sense to
ignore SWR for user initiated reloads.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2569: // cache_ is a weak pointer.
nit: move comment inline with the argument

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
net/http/http_response_headers.cc:1028: return Freshness(lifetime);
We have to look at must-revalidate before deciding if this could be usable.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
net/http/http_response_headers.cc:1043: return Freshness();
This may be usable

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
net/http/http_response_headers.cc:1086: return Freshness(TimeDelta::Max());
must-revalidate again

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
net/http/http_response_headers.cc:1089: return Freshness();
may be usable.

I think things are simpler if this becomes GetFreshnessLifetimes() and returns
two deltas instead of one.

The current structure of this method is to look at all relevant headers and come
up with _the_ delta when this resource is considered fresh (and keep
RequiresValidation  trivial).

Now we know that there could be an extra delta after that, when the resource may
be used. Given that we are looking at all other headers, except SWR, it makes
more sense to look at the new header and return the two values that
RequiresValidation would need.

Adam Rice

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#newcode412 net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(), transaction_.get()); On 2014/09/05 22:52:39, rvargas wrote: > On ...

6 years, 3 months ago (2014-09-09 12:36:46 UTC) #15

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:412: cache_->DoomEntry(transaction_->key(),
transaction_.get());
On 2014/09/05 22:52:39, rvargas wrote:
> On 2014/09/05 15:17:11, Adam Rice wrote:
> > On 2014/09/03 01:25:53, rvargas wrote:
> > > On 2014/09/01 14:43:00, Adam Rice wrote:
> > > > On 2014/08/29 23:36:06, rvargas wrote:
> > > > > why? Looks like a problem with this code, not with the entry.
> > > > 
> > > > Consider the case where the remote server suddenly asks for a client
> > > > certificate. With this design, there's no way we can satisfy the
request.
> > > > 
> > > > However, even in a different design where SSL certificate errors can be
> > > handled,
> > > > we shouldn't pop-up UI for an asynchronous request. So if we need to ask
> the
> > > > user to select a client certificate, we can't do it now, and the request
> > > should
> > > > fail.
> > > > 
> > > > The ideal behaviour is to make sure that if the user hits the same
> resource
> > > > again, the next time we make a synchronous request.
> > > > 
> > > > This could maybe be done by adding a flag to HttpResponseInfo, but for
the
> > > sake
> > > > of simplicity this initial implementation just dooms the entry.
> > > 
> > > We don't have to pop up an auth dialog for every resource that is loaded
> from
> > a
> > > given domain. The auth layer deals with that. I'm quite wary of deleting
> > entries
> > > because we don't have the right code in place... so either the code uses
> other
> > > code at the 'right' layer or it replicates the required logic.
> > > 
> > > Even on the final design, if an auth dialog is to be triggered with an
async
> > > request because no resource was required from the server before, I would
not
> > be
> > > very fond of deleting entries from the cache... even if that means that
the
> > > resource is not validated against the server and we have to wait until
some
> > auth
> > > credentials are in place.
> > 
> > For the final design, I'm thinking of something like having an
> > async_revalidation_failed flag in HttpResponseInfo that would prevent
entering
> > the async path for the same resource again. But that would mean a disk cache
> > format change, so it's not something I'd do lightly.
> 
> My suggestion for the final design would be that the revalidation would not be
> initiated by the cache, so whoever takes care of driving the revalidation
should
> decide what to do.
> 
> As for this version, I'm still not convinced of deleting the entry. At least
not
> without first trying to solve the problem.

The fact that another asynchronous validation should not be attempted will still
need to be stored somewhere, and the cache seems like the natural place.

I will try to wire up the certificate-error handling code to AsyncValidation but
it will be ugly and everyone will hate it so I will do it an a separate CL.

https://codereview.chromium.org/455623003/diff/280001/net/http/http_cache.cc#...
net/http/http_cache.cc:1180: DCHECK(insert_ok);
On 2014/09/05 22:52:39, rvargas wrote:
> On 2014/09/05 15:17:11, Adam Rice wrote:
> > On 2014/09/03 01:25:52, rvargas wrote:
> > > On 2014/09/01 14:43:00, Adam Rice wrote:
> > > > On 2014/08/29 23:36:06, rvargas wrote:
> > > > > why can't this fail?
> > > > 
> > > > It is matched with the erase() in DeleteAsyncValidation. new
> > AsyncValidation()
> > > > cannot return the same pointer until the old one is freed, and
> > > AsyncValidations
> > > > are only freed in DeleteAsyncValidation().
> > > > 
> > > > Strictly speaking, they are also freed in the ~HttpCache() destructor,
but
> > > that
> > > > calls InvalidateWeakPtrs() first, ensuring that PerformAsyncValidation()
> > > cannot
> > > > be called.
> > > 
> > > But we should not be performing two async validations for the same
resource
> at
> > > the same time, right? Shouldn't we ignore a second request if a another is
> in
> > > progress?
> > 
> > It might be possible to get multiple AsyncValidations to the same URL due to
a
> > race condition, but I haven't managed to get it to happen in my tests. All
> that
> > will happen is that they will all wait behind the cache lock for the first
one
> > to complete, and then (assuming the revalidation was successful) complete
from
> > the cache.
> > 
> > Anyway, the std::set is only comparing the pointers, it doesn't care whether
> > they refer to the same resource or not.
> 
> Sorry I was not clear. Yes, things will be queued by the lock (so there is no
> race), but that means consecutive revalidations are going through the wire.
> Clearly, the first revalidation should invalidate the others as the assumption
> that a revalidation is needed doesn't hold anymore.
> 
> So while I see why a DCHECK here (with the current implementation) should
never
> fire (and it is a good invariant check), I think we should be keeping track of
> the URL being revalidated and not queue another revalidation for the same
> resource.

Done.

https://codereview.chromium.org/455623003/diff/320001/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/455623003/diff/320001/net/base/load_flags_lis...
net/base/load_flags_list.h:130: // flag just prevents re-entering the async code
path, so you probably don't
On 2014/09/05 22:52:39, rvargas wrote:
> nit: The comment risks becoming stale without noticing. When we move issuing
the
> request above the cache there has to be a way to tell the cache not to serve
> stale content again, right?

I removed the comment. I'm still concerned about creating a false expectation
that it does something exciting and wasting people's time.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.cc#...
net/http/http_cache.cc:360:
transaction_.reset(static_cast<Transaction*>(transaction.release()));
On 2014/09/05 22:52:39, rvargas wrote:
> ... or, pass the transaction at construction...

DoomEntry() requires an HttpCache::Transaction. We could store the pointer as an
HttpTransaction and downcast it on the call to DoomEntry(), but to me it seems
safer to put the downcast close to the point of construction.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.h
File net/http/http_cache.h (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.h#n...
net/http/http_cache.h:454: // live AsyncValidation objects connected to this
cache are in this set.
On 2014/09/05 22:52:39, rvargas wrote:
> Isn't the invariant the expected behavior? Why document something that would
be
> extremely weird to violate?

I have seen people do a lot of weird things in my time.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2114: bool skip_validation =
required_validation == ValidationType::VALIDATION_NONE;
On 2014/09/05 22:52:39, rvargas wrote:
> nit: use () around ==

Done.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2308: return ValidationType::VALIDATION_NONE;
On 2014/09/05 22:52:40, rvargas wrote:
> I think we should move the enum out of ResponseHeaders. It clearly has to be
> used by other parts of the code when not interacting directly with response
> headers and it feels weird that load flags can be used as they are and
> validation type cannot. Besides, it is redundant to effectively have
> 'validation' twice in the name of each value.
> 
> (just to be clear, defining the enum in http_response_headers.h is fine)

Done.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2310: if (effective_load_flags_ &
(LOAD_VALIDATE_CACHE | LOAD_ASYNC_REVALIDATION))
On 2014/09/05 22:52:39, rvargas wrote:
> I just realized, can't we use LOAD_VALIDATE_CACHE instead of defining the new
> flag? This one doesn't add headers to blast proxies, and it also makes sense
to
> ignore SWR for user initiated reloads.

I tried that. It does add headers. See http_network_transaction.cc:880.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:2569: // cache_ is a weak pointer.
On 2014/09/05 22:52:40, rvargas wrote:
> nit: move comment inline with the argument

Done.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
net/http/http_response_headers.cc:1043: return Freshness();
On 2014/09/05 22:52:40, rvargas wrote:
> This may be usable

Acknowledged.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
net/http/http_response_headers.cc:1086: return Freshness(TimeDelta::Max());
On 2014/09/05 22:52:40, rvargas wrote:
> must-revalidate again

This never becomes stale, so must-revalidate is a no-op.

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
net/http/http_response_headers.cc:1089: return Freshness();
On 2014/09/05 22:52:40, rvargas wrote:
> may be usable.
> 
> I think things are simpler if this becomes GetFreshnessLifetimes() and returns
> two deltas instead of one.
> 
> The current structure of this method is to look at all relevant headers and
come
> up with _the_ delta when this resource is considered fresh (and keep
> RequiresValidation  trivial).
> 
> Now we know that there could be an extra delta after that, when the resource
may
> be used. Given that we are looking at all other headers, except SWR, it makes
> more sense to look at the new header and return the two values that
> RequiresValidation would need.

Done.

rvargas (doing something else)

https://codereview.chromium.org/455623003/diff/320001/net/base/load_flags_list.h File net/base/load_flags_list.h (right): https://codereview.chromium.org/455623003/diff/320001/net/base/load_flags_list.h#newcode130 net/base/load_flags_list.h:130: // flag just prevents re-entering the async code path, ...

6 years, 3 months ago (2014-09-11 02:33:07 UTC) #16

Adam Rice

https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/455623003/diff/320001/net/http/http_cache.cc#newcode360 net/http/http_cache.cc:360: transaction_.reset(static_cast<Transaction*>(transaction.release())); On 2014/09/11 02:33:05, rvargas wrote: > On 2014/09/09 ...

6 years, 3 months ago (2014-09-12 01:46:49 UTC) #17

rvargas (doing something else)

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_headers.cc#newcode1028 net/http/http_response_headers.cc:1028: return Freshness(lifetime); On 2014/09/12 01:46:48, Adam Rice wrote: > ...

6 years, 3 months ago (2014-09-12 18:33:13 UTC) #18

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
net/http/http_response_headers.cc:1028: return Freshness(lifetime);
On 2014/09/12 01:46:48, Adam Rice wrote:
> On 2014/09/11 02:33:06, rvargas wrote:
> > On 2014/09/05 22:52:40, rvargas wrote:
> > > We have to look at must-revalidate before deciding if this could be
usable.
> > 
> > ?
> 
> The existing code doesn't look at must-revalidate in this case. I don't want
to
> change behaviour.

must-revalidate doesn't matter for the current code as there is a clear signal
about when to consider the resource fresh. However, the new code extends that
clear signal with another one, saying that if it is not fresh, it still can be
used... that last part depends on must-revalidate so it has to be considered.

https://codereview.chromium.org/455623003/diff/340001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/455623003/diff/340001/chrome/common/chrome_sw...
chrome/common/chrome_switches.h:177: extern const char
kEnableStaleWhileRevalidate[];
On 2014/09/12 01:46:48, Adam Rice wrote:
> On 2014/09/11 02:33:06, rvargas wrote:
> > why 2 arguments?
> 
> I don't know. Some things have Enable/Disable pairs and some don't.
> 
> I don't know what criteria to use to make a decision.

We should not have two arguments with opposite meaning. I suspect sometimes we
end up with two because we start with a disable-by-default logic and later move
to enable-by-default without cleaning the code.

https://codereview.chromium.org/455623003/diff/340001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/340001/net/http/http_response_...
net/http/http_response_headers.cc:1081: // "must-revalidate" from influencing
heuristic expiration. This code should
On 2014/09/12 01:46:49, Adam Rice wrote:
> On 2014/09/11 02:33:06, rvargas wrote:
> > I'm missing the must-revalidate part of this comment
> 
> RFC7234 is missing the "based solely on the origin server's Expires or max-age
> value" part from RFC2616.
> 
> Actually, I'm not completely sure that RFC2616 supports Chrome's current
> must-revalidate behaviour, but maybe Chrome duplicates what other browsers do?
> 
> I also still don't know how stale-while-revalidate and must-revalidate
interact.
> I have been assuming stale-while-revalidate takes preference.

I don't see how the removal of that sentence changes this code. The way I see
it, it was removed because it really doesn't matter how the browser determines
that a resource is stale. But once it determines that it is stale, it MUST
revalidate with the server before using that stale resource (if there is a
must-revalidate header).

I think it is pretty clear that if a server sends must-revalidate, this spec
says that we should not use stale resources, and that includes other headers
that the server may have used to encourage use of stale resources.

In other words, there may be another header hinting at opposite behavior, but
the base HTTP spec is clearly normative (MUST)

https://codereview.chromium.org/455623003/diff/360001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/360001/net/http/http_cache.cc#...
net/http/http_cache.cc:427: // load_flags. This should be true for the lifetime
of this implementation.
breaking that invariant would mean breaking the definition of URI from the point
of view of the cache so it seems pretty safe to me.

If you decide to keep the comment, add an empty line above.

https://codereview.chromium.org/455623003/diff/360001/net/http/http_cache.cc#...
net/http/http_cache.cc:1211: net_log, downcast_transaction.Pass(),
network_delegate);
use PassAs()

https://codereview.chromium.org/455623003/diff/360001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/360001/net/http/http_response_...
net/http/http_response_headers.cc:1015: HasHeaderValue("vary", "*")) { // see
RFC 2616 section 13.6
nit: The comment location looks weird. Next line? (I know it's supposed to apply
only to this line). At least another space.

Adam Rice

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_headers.cc#newcode1028 net/http/http_response_headers.cc:1028: return Freshness(lifetime); On 2014/09/12 18:33:12, rvargas wrote: > On ...

6 years, 3 months ago (2014-09-16 11:53:01 UTC) #19

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/320001/net/http/http_response_...
net/http/http_response_headers.cc:1028: return Freshness(lifetime);
On 2014/09/12 18:33:12, rvargas wrote:
> On 2014/09/12 01:46:48, Adam Rice wrote:
> > On 2014/09/11 02:33:06, rvargas wrote:
> > > On 2014/09/05 22:52:40, rvargas wrote:
> > > > We have to look at must-revalidate before deciding if this could be
> usable.
> > > 
> > > ?
> > 
> > The existing code doesn't look at must-revalidate in this case. I don't want
> to
> > change behaviour.
> 
> must-revalidate doesn't matter for the current code as there is a clear signal
> about when to consider the resource fresh. However, the new code extends that
> clear signal with another one, saying that if it is not fresh, it still can be
> used... that last part depends on must-revalidate so it has to be considered.

Done.

https://codereview.chromium.org/455623003/diff/340001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/340001/net/http/http_response_...
net/http/http_response_headers.cc:1081: // "must-revalidate" from influencing
heuristic expiration. This code should
On 2014/09/12 18:33:12, rvargas wrote:
> On 2014/09/12 01:46:49, Adam Rice wrote:
> > On 2014/09/11 02:33:06, rvargas wrote:
> > > I'm missing the must-revalidate part of this comment
> > 
> > RFC7234 is missing the "based solely on the origin server's Expires or
max-age
> > value" part from RFC2616.
> > 
> > Actually, I'm not completely sure that RFC2616 supports Chrome's current
> > must-revalidate behaviour, but maybe Chrome duplicates what other browsers
do?
> > 
> > I also still don't know how stale-while-revalidate and must-revalidate
> interact.
> > I have been assuming stale-while-revalidate takes preference.
> 
> I don't see how the removal of that sentence changes this code. The way I see
> it, it was removed because it really doesn't matter how the browser determines
> that a resource is stale. But once it determines that it is stale, it MUST
> revalidate with the server before using that stale resource (if there is a
> must-revalidate header).
> 
> I think it is pretty clear that if a server sends must-revalidate, this spec
> says that we should not use stale resources, and that includes other headers
> that the server may have used to encourage use of stale resources.
> 
> In other words, there may be another header hinting at opposite behavior, but
> the base HTTP spec is clearly normative (MUST)

I removed the comment. If the meaning of RFC2616 and RFC7234 is the same with
respect to must-revalidate, then the check for must-revalidate in this if
statement makes no sense. However, I definitely don't want to change any
existing behaviour with this CL.

https://codereview.chromium.org/455623003/diff/360001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/455623003/diff/360001/net/http/http_cache.cc#...
net/http/http_cache.cc:427: // load_flags. This should be true for the lifetime
of this implementation.
On 2014/09/12 18:33:12, rvargas wrote:
> breaking that invariant would mean breaking the definition of URI from the
point
> of view of the cache so it seems pretty safe to me.
> 
> If you decide to keep the comment, add an empty line above.

I removed the comment.

https://codereview.chromium.org/455623003/diff/360001/net/http/http_cache.cc#...
net/http/http_cache.cc:1211: net_log, downcast_transaction.Pass(),
network_delegate);
On 2014/09/12 18:33:12, rvargas wrote:
> use PassAs()

PassAs() doesn't work for downcasts. I can't find a more elegant way to express
it.

https://codereview.chromium.org/455623003/diff/360001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/455623003/diff/360001/net/http/http_response_...
net/http/http_response_headers.cc:1015: HasHeaderValue("vary", "*")) { // see
RFC 2616 section 13.6
On 2014/09/12 18:33:12, rvargas wrote:
> nit: The comment location looks weird. Next line? (I know it's supposed to
apply
> only to this line). At least another space.

That didn't look right. I tried putting it above, with some extra words to
clarify which subexpression it refers to. PTAL.

rvargas (doing something else)

lgtm, thanks. https://codereview.chromium.org/455623003/diff/360001/net/http/http_response_headers.cc File net/http/http_response_headers.cc (right): https://codereview.chromium.org/455623003/diff/360001/net/http/http_response_headers.cc#newcode1015 net/http/http_response_headers.cc:1015: HasHeaderValue("vary", "*")) { // see RFC 2616 ...

6 years, 3 months ago (2014-09-17 04:32:20 UTC) #20

Adam Rice

https://codereview.chromium.org/455623003/diff/380001/net/http/http_cache_unittest.cc File net/http/http_cache_unittest.cc (right): https://codereview.chromium.org/455623003/diff/380001/net/http/http_cache_unittest.cc#newcode42 net/http/http_cache_unittest.cc:42: using base::RunLoop; On 2014/09/17 04:32:19, rvargas wrote: > nit: ...

6 years, 2 months ago (2014-09-30 13:44:51 UTC) #21

Adam Rice

I added support to HC::AsyncValidation for restarting the transaction for auth. I had to do ...

6 years, 2 months ago (2014-10-02 14:47:42 UTC) #22

Adam Rice

ricea@chromium.org changed reviewers: + asvitkine@chromium.org, mmenke@chromium.org, thestig@chromium.org

6 years, 2 months ago (2014-10-07 08:26:46 UTC) #24

Adam Rice

+asvitkine for histograms.xml (new histogram is logged at http_cache.cc:443) +mmenke for chrome/browser/io_thread.*, chrome/browser/net, chrome/browser/resources/net_internals/ +thestig ...

6 years, 2 months ago (2014-10-07 08:26:47 UTC) #25

Alexei Svitkine (slow)

lgtm % nit https://codereview.chromium.org/455623003/diff/460001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/455623003/diff/460001/tools/metrics/histograms/histograms.xml#newcode9822 tools/metrics/histograms/histograms.xml:9822: + The time spent doing HTTP ...

6 years, 2 months ago (2014-10-07 18:25:44 UTC) #26

Lei Zhang

chrome/browser/predictors/ lgtm, but please try someone in chrome/browser/predictors/OWNERS first next time.

6 years, 2 months ago (2014-10-07 18:56:20 UTC) #27

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/455623003/500001

6 years, 2 months ago (2014-10-08 01:38:19 UTC) #29

commit-bot: I haz the power

Committed patchset #26 (id:500001) as 0b4c8f92e4a0e40ae3824918eda7d3eb0075f688

6 years, 2 months ago (2014-10-08 03:37:12 UTC) #30

commit-bot: I haz the power

Patchset 26 (id:??) landed as https://crrev.com/64c07d7959386d30a0e4aa1371e2bdcd3d436476 Cr-Commit-Position: refs/heads/master@{#298649}

6 years, 2 months ago (2014-10-08 03:37:57 UTC) #31

Adam Rice

+mmenke Sorry, I didn't realise I was missing an lgtm. Do you have anything you'd ...

6 years, 2 months ago (2014-10-09 09:10:56 UTC) #32

mmenke

A couple nits. https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_thread.cc#newcode667 chrome/browser/io_thread.cc:667: globals_->use_stale_while_revalidate = true; nit: Use braces ...

6 years, 2 months ago (2014-10-09 14:29:58 UTC) #33

mmenke

Oh, and sorry about slowness - the entire network stack team is at an offsite. ...

6 years, 2 months ago (2014-10-09 14:30:49 UTC) #34

rvargas (doing something else)

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 14:29:58, mmenke wrote: > I ...

6 years, 2 months ago (2014-10-09 19:13:45 UTC) #35

mmenke

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 19:13:45, rvargas wrote: > On ...

6 years, 2 months ago (2014-10-09 19:32:01 UTC) #36

Message was sent while issue was closed.

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:57: base::TimeDelta stale;
On 2014/10/09 19:13:45, rvargas wrote:
> On 2014/10/09 14:29:58, mmenke wrote:
> > I think these names are very confusing - it doesn't imply they're deltas. 
> > "fresh" sounds like a bool to me "FreshnessLifetime" sounds like either an
> > expiration time, or total duration, neither of which is correct, so doesn't
> > really help.
> > 
> > Also, "fresh" and "stale" seem a bit confusing - generally something is
> > considered fresh until it's stale.
> > 
> > Maybe rename these:
> > fresh -> fresh_time_remaining
> > stale -> usable_time_remaining (Or stale_while_revalidate_time_remaining - a
> bit
> > of a mouthful, but clearer)
> > 
> > Or maybe time_until_stale / time_until_unusable?  I'm open to other ideas.
> 
> FreshnessLifetime is the standard term for time delta until the resource
becomes
> stale: http://tools.ietf.org/html/rfc7234#section-4.2.1 so it should provide
> enough context for the meaning of the members (maybe add a link to the rfc?).
> The idea is that now we compute two lifetimes instead of one.
>
> maybe fresh_delta or freshness_delta? I don't want to deviate too much from
the
> standard name.

Thanks for the link!  I guess this function only makes sense for for recently
received headers, and not those read from the cache, which means the time from
now until expiration is the same as freshness lifetime?

"Fresh" really does sounds like a bool to me - either something is fresh, or it
isn't, which initially confused me.  Since it's a defined term, as you point
out, maybe just freshness_lifetime?

> The second part is more complicated because from the point of view of the
> standard there are multiple conditions that allow use of stale data even
without
> considering SWR.
> 
> maybe stale_while_revalidate_delta?
> 
> I'm not very fond of adding part of the type name to the name of variables
> though.

Understandable - given my misunderstanding about when this method is used, I'm
fine with using something that just implies "not a bool", rather than
specifically "time delta".  stale_while_revalidate_until?

Anyhow, I'm not really too picky.

rvargas (doing something else)

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 19:32:01, mmenke wrote: > On ...

6 years, 2 months ago (2014-10-09 21:29:32 UTC) #37

Message was sent while issue was closed.

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:57: base::TimeDelta stale;
On 2014/10/09 19:32:01, mmenke wrote:
> On 2014/10/09 19:13:45, rvargas wrote:
> > On 2014/10/09 14:29:58, mmenke wrote:
> > > I think these names are very confusing - it doesn't imply they're deltas. 
> > > "fresh" sounds like a bool to me "FreshnessLifetime" sounds like either an
> > > expiration time, or total duration, neither of which is correct, so
doesn't
> > > really help.
> > > 
> > > Also, "fresh" and "stale" seem a bit confusing - generally something is
> > > considered fresh until it's stale.
> > > 
> > > Maybe rename these:
> > > fresh -> fresh_time_remaining
> > > stale -> usable_time_remaining (Or stale_while_revalidate_time_remaining -
a
> > bit
> > > of a mouthful, but clearer)
> > > 
> > > Or maybe time_until_stale / time_until_unusable?  I'm open to other ideas.
> > 
> > FreshnessLifetime is the standard term for time delta until the resource
> becomes
> > stale: http://tools.ietf.org/html/rfc7234#section-4.2.1 so it should provide
> > enough context for the meaning of the members (maybe add a link to the
rfc?).
> > The idea is that now we compute two lifetimes instead of one.
> >
> > maybe fresh_delta or freshness_delta? I don't want to deviate too much from
> the
> > standard name.
> 
> 
> Thanks for the link!  I guess this function only makes sense for for recently
> received headers, and not those read from the cache, which means the time from
> now until expiration is the same as freshness lifetime?

GetFreshnessLifetimes() is used by RequiresValidation() to calculate validation
requirements for cached content.

> 
> "Fresh" really does sounds like a bool to me - either something is fresh, or
it
> isn't, which initially confused me.  Since it's a defined term, as you point
> out, maybe just freshness_lifetime?

That works for me.

> 
> > The second part is more complicated because from the point of view of the
> > standard there are multiple conditions that allow use of stale data even
> without
> > considering SWR.
> > 
> > maybe stale_while_revalidate_delta?
> > 
> > I'm not very fond of adding part of the type name to the name of variables
> > though.
> 
> Understandable - given my misunderstanding about when this method is used, I'm
> fine with using something that just implies "not a bool", rather than
> specifically "time delta".  stale_while_revalidate_until?
> 

"until" suggests to me an absolute value (base::Time) :(

Maybe stale_while_revalidate_lifetime? stale_while_revalidate_value?

> Anyhow, I'm not really too picky.

Alexei Svitkine (slow)

https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_thread.cc#newcode663 chrome/browser/io_thread.cc:663: if (command_line.HasSwitch(switches::kEnableStaleWhileRevalidate)) I didn't review this part earlier, but ...

6 years, 2 months ago (2014-10-09 21:33:39 UTC) #38

rvargas (doing something else)

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 21:29:32, rvargas wrote: > On ...

6 years, 2 months ago (2014-10-09 21:47:14 UTC) #39

Message was sent while issue was closed.

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:57: base::TimeDelta stale;
On 2014/10/09 21:29:32, rvargas wrote:
> On 2014/10/09 19:32:01, mmenke wrote:
> > On 2014/10/09 19:13:45, rvargas wrote:
> > > On 2014/10/09 14:29:58, mmenke wrote:
> > > > I think these names are very confusing - it doesn't imply they're
deltas. 
> > > > "fresh" sounds like a bool to me "FreshnessLifetime" sounds like either
an
> > > > expiration time, or total duration, neither of which is correct, so
> doesn't
> > > > really help.
> > > > 
> > > > Also, "fresh" and "stale" seem a bit confusing - generally something is
> > > > considered fresh until it's stale.
> > > > 
> > > > Maybe rename these:
> > > > fresh -> fresh_time_remaining
> > > > stale -> usable_time_remaining (Or stale_while_revalidate_time_remaining
-
> a
> > > bit
> > > > of a mouthful, but clearer)
> > > > 
> > > > Or maybe time_until_stale / time_until_unusable?  I'm open to other
ideas.
> > > 
> > > FreshnessLifetime is the standard term for time delta until the resource
> > becomes
> > > stale: http://tools.ietf.org/html/rfc7234#section-4.2.1 so it should
provide
> > > enough context for the meaning of the members (maybe add a link to the
> rfc?).
> > > The idea is that now we compute two lifetimes instead of one.
> > >
> > > maybe fresh_delta or freshness_delta? I don't want to deviate too much
from
> > the
> > > standard name.
> > 
> > 
> > Thanks for the link!  I guess this function only makes sense for for
recently
> > received headers, and not those read from the cache, which means the time
from
> > now until expiration is the same as freshness lifetime?
> 
> GetFreshnessLifetimes() is used by RequiresValidation() to calculate
validation
> requirements for cached content.
> 
> > 
> > "Fresh" really does sounds like a bool to me - either something is fresh, or
> it
> > isn't, which initially confused me.  Since it's a defined term, as you point
> > out, maybe just freshness_lifetime?
> 
> That works for me.
> 
> > 
> > > The second part is more complicated because from the point of view of the
> > > standard there are multiple conditions that allow use of stale data even
> > without
> > > considering SWR.
> > > 
> > > maybe stale_while_revalidate_delta?
> > > 
> > > I'm not very fond of adding part of the type name to the name of variables
> > > though.
> > 
> > Understandable - given my misunderstanding about when this method is used,
I'm
> > fine with using something that just implies "not a bool", rather than
> > specifically "time delta".  stale_while_revalidate_until?
> > 
> 
> "until" suggests to me an absolute value (base::Time) :(
> 
> Maybe stale_while_revalidate_lifetime? stale_while_revalidate_value?
> 
> > Anyhow, I'm not really too picky.
> 

BTW, another thing to consider is how the code reads when it is used.

Today this means
  lifetime.fresh.InSeconds(),
  lifetime.stale.InSeconds(),

and
  TimeDelta freshness_lifetime =
     foo->GetFreshnessLifetimes(blah).fresh;

crazy idea: 
  lifetime.freshness.InSeconds(),
  lifetime.staleness.InSeconds(),

on the upside (and downside), I don't think "staleness" means anything :p

mmenke

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h File net/http/http_response_headers.h (right): https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_headers.h#newcode57 net/http/http_response_headers.h:57: base::TimeDelta stale; On 2014/10/09 21:47:14, rvargas wrote: > On ...

6 years, 2 months ago (2014-10-09 21:50:28 UTC) #40

Message was sent while issue was closed.

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:57: base::TimeDelta stale;
On 2014/10/09 21:47:14, rvargas wrote:
> On 2014/10/09 21:29:32, rvargas wrote:
> > On 2014/10/09 19:32:01, mmenke wrote:
> > > On 2014/10/09 19:13:45, rvargas wrote:
> > > > On 2014/10/09 14:29:58, mmenke wrote:
> > > > > I think these names are very confusing - it doesn't imply they're
> deltas. 
> > > > > "fresh" sounds like a bool to me "FreshnessLifetime" sounds like
either
> an
> > > > > expiration time, or total duration, neither of which is correct, so
> > doesn't
> > > > > really help.
> > > > > 
> > > > > Also, "fresh" and "stale" seem a bit confusing - generally something
is
> > > > > considered fresh until it's stale.
> > > > > 
> > > > > Maybe rename these:
> > > > > fresh -> fresh_time_remaining
> > > > > stale -> usable_time_remaining (Or
stale_while_revalidate_time_remaining
> -
> > a
> > > > bit
> > > > > of a mouthful, but clearer)
> > > > > 
> > > > > Or maybe time_until_stale / time_until_unusable?  I'm open to other
> ideas.
> > > > 
> > > > FreshnessLifetime is the standard term for time delta until the resource
> > > becomes
> > > > stale: http://tools.ietf.org/html/rfc7234#section-4.2.1 so it should
> provide
> > > > enough context for the meaning of the members (maybe add a link to the
> > rfc?).
> > > > The idea is that now we compute two lifetimes instead of one.
> > > >
> > > > maybe fresh_delta or freshness_delta? I don't want to deviate too much
> from
> > > the
> > > > standard name.
> > > 
> > > 
> > > Thanks for the link!  I guess this function only makes sense for for
> recently
> > > received headers, and not those read from the cache, which means the time
> from
> > > now until expiration is the same as freshness lifetime?
> > 
> > GetFreshnessLifetimes() is used by RequiresValidation() to calculate
> validation
> > requirements for cached content.
> > 
> > > 
> > > "Fresh" really does sounds like a bool to me - either something is fresh,
or
> > it
> > > isn't, which initially confused me.  Since it's a defined term, as you
point
> > > out, maybe just freshness_lifetime?
> > 
> > That works for me.
> > 
> > > 
> > > > The second part is more complicated because from the point of view of
the
> > > > standard there are multiple conditions that allow use of stale data even
> > > without
> > > > considering SWR.
> > > > 
> > > > maybe stale_while_revalidate_delta?
> > > > 
> > > > I'm not very fond of adding part of the type name to the name of
variables
> > > > though.
> > > 
> > > Understandable - given my misunderstanding about when this method is used,
> I'm
> > > fine with using something that just implies "not a bool", rather than
> > > specifically "time delta".  stale_while_revalidate_until?
> > > 
> > 
> > "until" suggests to me an absolute value (base::Time) :(
> > 
> > Maybe stale_while_revalidate_lifetime? stale_while_revalidate_value?
> > 
> > > Anyhow, I'm not really too picky.
> > 
> 
> BTW, another thing to consider is how the code reads when it is used.
> 
> Today this means
>   lifetime.fresh.InSeconds(),
>   lifetime.stale.InSeconds(),
> 
> and
>   TimeDelta freshness_lifetime =
>      foo->GetFreshnessLifetimes(blah).fresh;
> 
> crazy idea: 
>   lifetime.freshness.InSeconds(),
>   lifetime.staleness.InSeconds(),
> 
> on the upside (and downside), I don't think "staleness" means anything :p

I was looking at it in chrome_network_delegate.cc..By the time I'd gotten to the
end of that line, I'd forgotten about what the start looked like.

Adam Rice

6 years, 2 months ago (2014-10-20 09:31:24 UTC) #41

Message was sent while issue was closed.

https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:663: if
(command_line.HasSwitch(switches::kEnableStaleWhileRevalidate))
On 2014/10/09 21:33:39, Alexei Svitkine wrote:
> I didn't review this part earlier, but now that I see this, I suggest using
the
> pattern suggested at go/finch-and-flags (where you assign the value from the
> field trial to a local first, so that the trial is activated and then check
the
> command-line, followed by the group comparison).

Done in https://crrev.com/665023002/

https://codereview.chromium.org/455623003/diff/500001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:667: globals_->use_stale_while_revalidate = true;
On 2014/10/09 14:29:58, mmenke wrote:
> nit:  Use braces when the conditional of an if statement takes more than one
> line.

Indirectly fixed by change requested by asvitkine.

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:32: VALIDATION_NONE = 0,      // The resource
is fresh.
On 2014/10/09 14:29:58, mmenke wrote:
> Does this "= 0" give us anything?

No. But all the cool kids are doing it.

Removed in https://crrev.com/665023002/

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:57: base::TimeDelta stale;
On 2014/10/09 21:50:28, mmenke wrote:
> On 2014/10/09 21:47:14, rvargas wrote:
> > On 2014/10/09 21:29:32, rvargas wrote:
> > > On 2014/10/09 19:32:01, mmenke wrote:
> > > > On 2014/10/09 19:13:45, rvargas wrote:
> > > > > On 2014/10/09 14:29:58, mmenke wrote:
> > > > > > I think these names are very confusing - it doesn't imply they're
> > deltas. 
> > > > > > "fresh" sounds like a bool to me "FreshnessLifetime" sounds like
> either
> > an
> > > > > > expiration time, or total duration, neither of which is correct, so
> > > doesn't
> > > > > > really help.
> > > > > > 
> > > > > > Also, "fresh" and "stale" seem a bit confusing - generally something
> is
> > > > > > considered fresh until it's stale.
> > > > > > 
> > > > > > Maybe rename these:
> > > > > > fresh -> fresh_time_remaining
> > > > > > stale -> usable_time_remaining (Or
> stale_while_revalidate_time_remaining
> > -
> > > a
> > > > > bit
> > > > > > of a mouthful, but clearer)
> > > > > > 
> > > > > > Or maybe time_until_stale / time_until_unusable?  I'm open to other
> > ideas.
> > > > > 
> > > > > FreshnessLifetime is the standard term for time delta until the
resource
> > > > becomes
> > > > > stale: http://tools.ietf.org/html/rfc7234#section-4.2.1 so it should
> > provide
> > > > > enough context for the meaning of the members (maybe add a link to the
> > > rfc?).
> > > > > The idea is that now we compute two lifetimes instead of one.
> > > > >
> > > > > maybe fresh_delta or freshness_delta? I don't want to deviate too much
> > from
> > > > the
> > > > > standard name.
> > > > 
> > > > 
> > > > Thanks for the link!  I guess this function only makes sense for for
> > recently
> > > > received headers, and not those read from the cache, which means the
time
> > from
> > > > now until expiration is the same as freshness lifetime?
> > > 
> > > GetFreshnessLifetimes() is used by RequiresValidation() to calculate
> > validation
> > > requirements for cached content.
> > > 
> > > > 
> > > > "Fresh" really does sounds like a bool to me - either something is
fresh,
> or
> > > it
> > > > isn't, which initially confused me.  Since it's a defined term, as you
> point
> > > > out, maybe just freshness_lifetime?
> > > 
> > > That works for me.
> > > 
> > > > 
> > > > > The second part is more complicated because from the point of view of
> the
> > > > > standard there are multiple conditions that allow use of stale data
even
> > > > without
> > > > > considering SWR.
> > > > > 
> > > > > maybe stale_while_revalidate_delta?
> > > > > 
> > > > > I'm not very fond of adding part of the type name to the name of
> variables
> > > > > though.
> > > > 
> > > > Understandable - given my misunderstanding about when this method is
used,
> > I'm
> > > > fine with using something that just implies "not a bool", rather than
> > > > specifically "time delta".  stale_while_revalidate_until?
> > > > 
> > > 
> > > "until" suggests to me an absolute value (base::Time) :(
> > > 
> > > Maybe stale_while_revalidate_lifetime? stale_while_revalidate_value?
> > > 
> > > > Anyhow, I'm not really too picky.
> > > 
> > 
> > BTW, another thing to consider is how the code reads when it is used.
> > 
> > Today this means
> >   lifetime.fresh.InSeconds(),
> >   lifetime.stale.InSeconds(),
> > 
> > and
> >   TimeDelta freshness_lifetime =
> >      foo->GetFreshnessLifetimes(blah).fresh;
> > 
> > crazy idea: 
> >   lifetime.freshness.InSeconds(),
> >   lifetime.staleness.InSeconds(),
> > 
> > on the upside (and downside), I don't think "staleness" means anything :p
> 
> I was looking at it in chrome_network_delegate.cc..By the time I'd gotten to
the
> end of that line, I'd forgotten about what the start looked like.

I have implemented the freshness/staleness approach in
https://crrev.com/665023002/

Just to complicate things, I thought of another possible naming strategy:

UsableLifetime usable_lifetime = headers.GetUsableLifetime(...);
usable_lifetime.as_fresh.InSeconds(),
usable_lifetime.as_stale.InSeconds()

https://codereview.chromium.org/455623003/diff/500001/net/http/http_response_...
net/http/http_response_headers.h:234: // without revalidation, and
|FreshnessLifetimes.stale| to the length of time
On 2014/10/09 14:29:58, mmenke wrote:
> nit:  Only use || around argument names.  I documenting FreshnessLifetime's
> fields again here isn't needed, since it's redundant with the struct - suspect
> the documentation here is much more likely to become outdated if things ever
> change.  Fine to mention RFC5861, and the usable while stale behavior.

Done.

Issue 455623003: stale-while-revalidate experimental implementation. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Implement HttpCache parts of asynchronous validation. #

Patch Set 3 : Separate NetLog source for async revalidation and add tests. #

Patch Set 4 : Rebase. #

Patch Set 5 : Add tests for HEAD and POST methods and to check the URLs match. #

Patch Set 6 : Add tests for load flags suppressing async revalidation and certificate failure causing synchronous… #

Patch Set 7 : clang-format fix. #

Patch Set 8 : Rebase. #

Patch Set 9 : Add new flags to histograms.xml #

Patch Set 10 : Fixes from yhirano review. #

Patch Set 11 : Renames RequiresValidation() to RequiredValidationType() #

Patch Set 12 : Add HttpCache.AsyncValidationTimeSaved histogram. #

Patch Set 13 : Support the bandwidth reduction proxy. #

Patch Set 14 : Ensure |lifetime| is always cleared in GetFreshnessLifetime() #

Patch Set 15 : Stop setting Cache-Control: max-age=0 on async revalidations. #

Patch Set 16 : Clean up GetFreshnessLifetime() miscellaneous other things. #

Patch Set 17 : Add load flag LOAD_ASYNC_REVALIDATION. #

Patch Set 18 : GetFreshnessLifetimes now returns two times. Duplicate AsyncValidations for the same URL are discar… #

Patch Set 19 : Cleanups. #

Patch Set 20 : Remove --disable-stale-while-revalidate flag and strengthen must-revalidate. #

Patch Set 21 : Unit test fixes. #

Patch Set 22 : Add support for restarting a transaction for auth. #

Patch Set 23 : Rebase. #

Patch Set 24 : Compile fix for ResourcePrefreshPredictor. #

Patch Set 25 : Clarify the summary for the histogram. #

Patch Set 26 : Rebase. #

Messages