Improved memcached module

[Maxim Dounin]

Hello!

On Fri, Aug 21, 2009 at 11:04:23AM +0200, Spil Games wrote:

> Maxim Dounin wrote:
> > There are at least 2 problems with this patch I see:
> > 
> > 1. It relies on 'HTTP/' prefix on to find out if key in question
> > has headers.  This is bad, as there is no way to return text
> > starting from this string.  IMHO it should be changed to use flags
> > instead.
> > 
> > 2. It changes u->conf during request execution, this implies race
> > between different requests.
> 
> Great feedback. I understand the 'HTTP/' prefix thing, the flags thing 
> is a good idea. I'll look into this.

Just a quick note: Tomash Brechko's memcached_gzip patches should 
have flags parsing code.  Code should be available via 
git.openhack.ru in nginx-patched.git, but it's awfully slow and 
I'm too lazy to search for exact link.

> Could you elaborate on the u->conf thing, because I'm not not at all an 
> Nginx expert. The patch basically consists of things I snatched from 
> other parts of Nginx (mostly from the proxy/fastcgi modules).

I mean here:

% +        /* disallow interception of error pages */
% +        u->conf->intercept_404 = 0;
% +        u->conf->intercept_errors = 0;
% +        r->error_page = 1;
% 
%          return NGX_OK;
%      }
% @@ -385,6 +563,8 @@
% 
%          u->headers_in.status_n = 404;
%          u->state->status = 404;
% +        u->conf->intercept_404 = 1;
% +        u->conf->intercept_errors = 1;
% 
%          return NGX_OK;
%      }

(btw, please use -p switch for diff, it simplifies reading patches 
a lot)

You modified u->conf->intercept_404 and u->conf->intercept_errors 
for this particular request.  Meanwhile u->conf is structure 
shared between all requests to this location (it's configuration, 
and supposed to be identical for all requests).

Looking closer - it should work without chance for any errors as 
of now, since this is done just before it's used in 
ngx_http_upstream.c and there is no way how nginx could switch to 
other request between setting them and checking (and threads in 
nginx are broken now anyway).

Nevertheless I think it should be fixed somehow (e.g. by 
duplicating relevant flags in request-specific data).

Maxim Dounin