Hello!

On Mon, Sep 26, 2011 at 11:10:07AM +1000, John Ferlito wrote:

>
> Howdy,
>
> I'd like to confirm what I think is a bug before I lodge it in the bug
> tracker.
>
> I'm seeing pages that shouldn't be cached be cached.
>
> I have a config that looks something like
>
>
> upstream app_pool {
> server app.inodes.org;
> fair;
> }
>
> proxy_cache_path /srv/www/reverse_proxy/cache levels=1:2 keys_zone=application_cache:8m max_size=1000m inactive=600m;
> proxy_temp_path /srv/www/reverse_proxy/cache/tmp;
>
> server {
> listen 80;
> server_name inodes.org;
>
> location / {
> proxy_pass http://app_pool;
> proxy_pass_header Set-Cookie;
>
> proxy_cache application_cache;
> proxy_cache_key "$scheme$host$request_uri$query_string";
>
> proxy_set_header Host $host;
> proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
> proxy_set_header X-Real-IP $remote_addr;
>
> proxy_cache_valid 200 302 301 60m;
> proxy_cache_valid 404 1m;
>
> proxy_cache_bypass $http_pragma;
> }
> }
>
>
> Now the behaviour I'm seeing is specifically for responses with a header like
>
> Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
>
> That is they shouldn't be cached. They only get cached after a request with
>
> Pragma: no-cache
>
> set.
>
> If I create a cache log I see something like this.
>
>
> MISS "GET /mospace/team/ HTTP/1.1" (200) # First load
> BYPASS "GET /mospace/team/ HTTP/1.1" (200) # Shift Reload
> HIT "GET /mospace/team/ HTTP/1.1" (200) # Standard load
>
> I've done some following of the code path and I think I understand
> where the bug lies.
>
> In ngx_http_upstream_cache.c we essentially have the following code (snipped
> for clarity)
>
> static void
> ngx_http_upstream_send_response(ngx_http_request_t *r, ngx_http_upstream_t *u)
> {
> switch (ngx_http_test_predicates(r, u->conf->no_cache)) {
>
> case NGX_DECLINED:
> u->cacheable = 0;
> break;
>
> default: /* NGX_OK */
>
> if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {
>
> r->cache->min_uses = u->conf->cache_min_uses;
> r->cache->body_start = u->conf->buffer_size;
> r->cache->file_cache = u->conf->cache->data;
>
> if (ngx_http_file_cache_create(r) != NGX_OK) {
> ngx_http_upstream_finalize_request(r, u, 0);
> return;
> }
>
> u->cacheable = 1;
> }
>
> break;
> }
> }
>
>
> So from my understanding if cache_bypass has been set we mark the page as being
> able to be cached.
>
> Now this function is called in ngx_http_upstream_process_header, which does the
> following
>
> if (ngx_http_upstream_process_headers(r, u) != NGX_OK) {
> return;
> }
>
> if (!r->subrequest_in_memory) {
> ngx_http_upstream_send_response(r, u);
> return;
> }
>
>
> From what I can tell ngx_http_upstream_process_headers will go through
> the headers and when it sees the appropriate Cache-Control header it
> will set cacheable to 0.
>
> But later on in ngx_http_upstream_send_response we are setting it to
> 1. Hence we start caching private pages but only when a bypass header
> has been set.

Ack, this is clearly a bug.

> Perhaps we should set cacheable to 1 much earlier in the process when
> BYPASS is set? That is in ngx_http_upstream_cache
>
> switch (ngx_http_test_predicates(r, u->conf->cache_bypass)) {
> // snip
> case NGX_DECLINED:
> u->cache_status = NGX_HTTP_CACHE_BYPASS;
> u-> cachable = 1; // Add this here maybe?
> return NGX_DECLINED;
> }
>
> This is the first time I've looked at the nginx source so perhaps the
> above would have some other unintended effect. If not let me know and
> I'll submit a patch.

As far as I see moving

u->cacheable = 1;

above the ngx_http_test_predicates(cache_bypass) check should be
ok.

Obviously "u->cacheable = 1" should be removed from
ngx_http_test_predicates(no_cache) switch.

Maxim Dounin

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel