[PATCH] Upstream: prioritise Cache-Control over Expires

[Vadim Fedorenko]

Hi Maxim!

Thanks for the feedback. As you might have already seen, I sent another version
of this patch which addresses some of style and readability issues as well as
X-Accel-Expires issue in v3.

Regarding RFC issue. RFC 7234 Section 5.1 explicitly states that Expires MUST be
ignored if reponse contains Cache-Control header with max-age or s-maxage
directives. That's why I said that Nginx server configured to cache responses
and to rely on headers to determine the freshness violates RFC. And this is true
for a subset of absolutely legitimate responses, and it was done intended to
actually optimize the internal processing but not because of configuration
optimizations.

Hope you will have some time to review version 3 of my patch even though it
might not clearly apply on top of the patchset you sent couple of days ago.

Thanks,
Vadim

On 22.04.2022 19:23, Maxim Dounin wrote:
> Hello!
> 
> On Tue, Apr 19, 2022 at 10:59:12PM +0100, Vadim Fedorenko wrote:
> 
>> On 19.04.2022 16:01, Maxim Dounin wrote:
>>>
>>> On Sun, Apr 17, 2022 at 08:50:25PM +0100, Vadim Fedorenko via nginx-devel wrote:
>>>
>>>> On 17.04.2022 02:55, Maxim Dounin wrote:
>>>>
>>>>> I'm quite sceptical about attempts to fix this by introducing
>>>>> various flags and reverting cacheable status back to 1.  This is
>>>>> not how it should be fixed.
>>>>
>>>> Yeah, the solution might be a bit complicated, but I couldn't find
>>>> another way without breaking concept of independent header parsing.
>>>> Could you please suggest something if you think that current approach
>>>> is wrong? The ticket that this patch tries to fix is 6 years old and
>>>> still has discussions going on without any solution.
>>>
>>> That's basically why the ticket is still not fixed: there is no
>>> easy fix, and the issue is rather minor, especially compared to
>>> the complexity of the changes required to fix it.
>>>
>> Hmm.. I would argue on the minority of the issue, especially when we talk about
>> absence of compatibility with RFC and actual discussions in the ticket. Anyway.
> 
> While current behaviour might be suboptimal in some use cases, 34it
> does not violate RFC, as caching is not something required to
> happen.  So we are talking about optimizing some use cases here,
> and the same optimization can be easily obtained by adjusting
> headers in the backend response.
> 
>>> IMHO, the most promising approach would be to move
>>> Cache-Control/Exires/X-Accel-Expires handling elsewhere, and
>>> handle them somewhere in ngx_http_upstream_process_headers(),
>>> probably along with ngx_http_file_cache_valid() as well.
>>>
>> Ok, cool, I updated my patch, now it's based on the work from Yugo that was left
>> without comments and is following the way you suggest. Take a look at v2, please.
> 
> Apart multiple style issues, the patch fails to address the
> X-Accel-Expires vs. Cache-Control order issue as outlined in the
> ticket, that is, the response with:
> 
> : X-Accel-Expires: 10
> : Cache-Control: max-age=100, stale-while-revalidate=1000
> 
> results in different behaviour than the one with:
> 
> : Cache-Control: max-age=100, stale-while-revalidate=1000
> : X-Accel-Expires: 10
> 
> It is also seems to be completely unreadable.  (Yugo's patch tried
> to address this, though had multiple style and readability issues
> as well.)
>