On 04/06/2007 01:13 PM, Georg von Zezschwitz wrote: > Jim Jagielski wrote: > >>> >>> If we state that the evaluation takes place in the occurence of >>> stickysession attributes >>> and suggest >>> >>> stickysession=Cookie:JSESSIONID stickysession=Path:;jsessionid to >>> the user >>> >>> it will perform faster in average. >>> >>> As I promised to write the patch, I would do it the way Jim suggested >>> it for backward >>> compatibility, however I would also add a table "sticky_type" >>> containing the "Cookie vs. >>> Path vs. Env"-prefix (in a parsed way) to the balancer struct. >>> >>> Is this OK for everybody? >>> >> >> Patch for trunk... > > > Hi, > > attached is the patch for trunk with documentation & Co. > > Could anybody review it & commit?
Many thanks for sending the patch and my apologies that reviewing it took that long. Please find my comments below. Regards Rüdiger Throughout the code there are a number of code style issues that should be fixed. Most of them are related to missing spaces before or after assignments or operators. I do not mention them explicitly in my review later. Please have a look at http://httpd.apache.org/dev/styleguide.html. Index: docs/manual/mod/mod_proxy.xml =================================================================== --- docs/manual/mod/mod_proxy.xml (revision 526077) +++ docs/manual/mod/mod_proxy.xml (working copy) @@ -707,9 +707,19 @@ </td></tr> <tr><td>stickysession</td> <td>-</td> - <td>Balancer sticky session name. The value is usually set to something - like <code>JSESSIONID</code> or <code>PHPSESSIONID</code>, - and it depends on the backend application server that support sessions. + <td>Balancer sticky session name. Multiple definitions are possible and + are interpreted in the sequence of their definition. The + value can be preceeded with a specification <code>Cookie</code>, + <code>Path</code> or <code>Env</code> to restrict the search. + Without this specification, the identifier is searched in the URL path + and then in the cookie. The value is usually set to something + like <code>JSESSIONID</code> or <code>PHPSESSIONID</code>, e.g. + <br/><indent> + <code>stickysession=Cookie:JSESSIONID + stickysession=Path:;jsessionid</code> + </indent> + for Java Servlets and depends on the backend application server + that support sessions. I think the examples should be split into two separate sentences: One for the simple case (no specification) for which PHPSESSIONID is a good example and one for the more complex case (two settings with different specifications) for which Java Servlets are a good example. Index: modules/proxy/mod_proxy_balancer.c =================================================================== --- modules/proxy/mod_proxy_balancer.c (revision 526077) +++ modules/proxy/mod_proxy_balancer.c (working copy) @@ -581,6 +605,30 @@ } } +static void print_balancer_sticky(request_rec *r, + apr_array_header_t *sticky) +{ + int i; + struct proxy_sticky_entry *entries; + + if (sticky==NULL) + return; + entries = (struct proxy_sticky_entry *) sticky->elts; + for (i = 0; i < sticky->nelts; i++) { + if (i>0) + ap_rputs (" ", r); + switch (entries[i].method) { + case PROXY_STICKY_COOKIE: + ap_rputs ("Cookie:", r); break; + case PROXY_STICKY_PATH: + ap_rputs ("Path:", r); break; + case PROXY_STICKY_ENV: + ap_rputs ("Env:", r); break; Style + } + ap_rputs (entries[i].name, r); + } +} + /* Manages the loadfactors and member status */ static int balancer_handler(request_rec *r) @@ -645,10 +693,30 @@ if (bsel) { const char *val; if ((val = apr_table_get(params, "ss"))) { - if (strlen(val)) - bsel->sticky = apr_pstrdup(conf->pool, val); - else - bsel->sticky = NULL; + char *tok_cntx; + bsel->sticky = apr_array_make(conf->pool, 1, + sizeof (struct proxy_sticky_entry)); + char *v = apr_strtok(apr_pstrdup(r->pool, val), "+", &tok_cntx); This is not allowed in ANSI C: Declarations of variables need to be at the beginning of a block. Better use ap_strchr here. + while (v!=NULL) { + struct proxy_sticky_entry * entry = + (struct proxy_sticky_entry *) apr_array_push(bsel->sticky); + const char *colon_pos = strchr (v, ':'); + entry->method = PROXY_STICKY_COOKIE | PROXY_STICKY_PATH; + if (colon_pos!=NULL) { + while (*++colon_pos==' ') + continue; + entry->name = apr_pstrdup(conf->pool, colon_pos); + if (!strncasecmp ("cookie:", v, 7)) + entry->method = PROXY_STICKY_COOKIE; + else if (!strncasecmp ("path:", v, 5)) + entry->method = PROXY_STICKY_PATH; + else if (!strncasecmp ("env:", v, 4)) + entry->method = PROXY_STICKY_ENV; + } else { + entry->name = apr_pstrdup(conf->pool, v); + } + v = apr_strtok(NULL, "+", &tok_cntx); + } } if ((val = apr_table_get(params, "tm"))) { int ival = atoi(val); Index: modules/proxy/mod_proxy.c =================================================================== --- modules/proxy/mod_proxy.c (revision 526077) +++ modules/proxy/mod_proxy.c (working copy) @@ -272,14 +272,36 @@ const char *key, const char *val) { - int ival; if (!strcasecmp(key, "stickysession")) { - /* Balancer sticky session name. - * Set to something like JSESSIONID or - * PHPSESSIONID, etc.., - */ - balancer->sticky = apr_pstrdup(p, val); + /* Balancer sticky session name. + * Set to something like JSESSIONID or + * PHPSESSIONID, etc.., + */ + struct proxy_sticky_entry *entry; + + if (balancer->sticky==NULL) + balancer->sticky = + apr_array_make(p, 1, sizeof (struct proxy_sticky_entry)); + entry = (struct proxy_sticky_entry *) apr_array_push(balancer->sticky); + const char *colon_pos = strchr (val, ':'); This is not allowed in ANSI C: Declarations of variables need to be at the beginning of a block. Better use ap_strchr_c here. + if (colon_pos!=NULL) { + while (*++colon_pos==' ') + continue; + entry->name = apr_pstrdup(p, colon_pos); + if (!strncasecmp ("cookie:", val, 7)) + entry->method = PROXY_STICKY_COOKIE; + else if (!strncasecmp ("path:", val, 5)) + entry->method = PROXY_STICKY_PATH; + else if (!strncasecmp ("env:", val, 4)) + entry->method = PROXY_STICKY_ENV; + else + return "stickysession must be preceeded by Cookie|Path|Env " + "followed by a colon"; + } else { + entry->method = PROXY_STICKY_COOKIE | PROXY_STICKY_PATH; + entry->name = apr_pstrdup(p, val); + } This looks very similar to the logic in mod_proxy_balancer.c. Does it make sense to duplicate the code? Shouldn't this be moved to proxy_util.c? @@ -324,7 +346,7 @@ return "unknown lbmethod"; } else { - return "unknown Balancer parameter"; + return apr_pstrcat (p, "unknown Balancer parameter ", key); The last parameter to apr_pstrcat needs to be NULL. } return NULL; } @@ -1152,7 +1173,8 @@ } else *val++ = '\0'; - apr_table_setn(params, word, val); >From my personal point of view I would prefer staying with the table and just do apr_table_addn when it comes to stickysession to have this key in the table multiple times. + * (const char**) (apr_array_push (params)) = word; + * (const char**) (apr_array_push (params)) = val; This style looks nasty and may break on some compilers. Please use a temporary variable to save the return value of apr_array_push. } }; @@ -1177,9 +1198,20 @@ if (err) return apr_pstrcat(cmd->temp_pool, "ProxyPass ", err, NULL); } - for (i = 0; i < arr->nelts; i++) { - const char *err = set_balancer_param(conf, cmd->pool, balancer, elts[i].key, - elts[i].val); + else { + if (params->nelts > 0 && + (balancer->sticky || balancer->sticky_force || + balancer->max_attempts_set || balancer->timeout)) { + ap_log_error(APLOG_MARK, APLOG_WARNING, 0, cmd->server, + "repeated parameter definition for balancer %s " + "overwrites previous definitions", + balancer->name); + balancer->sticky = NULL; Why resetting balancer->sticky here? It is not clear that the contents of balancer->sticky is changed. It makes sense to reset it if we have at least one "stickysession" parameter inside of params->elts so that we cleanly overwrite all the "stickysession" stuff then. Index: modules/proxy/mod_proxy.h =================================================================== --- modules/proxy/mod_proxy.h (revision 526077) +++ modules/proxy/mod_proxy.h (working copy) @@ -373,6 +373,15 @@ void *context; /* general purpose storage */ }; +struct proxy_sticky_entry { + const char *name; /* key to look for, e.g. JSESSIONID */ + int method; /* where to search, e.g. PROXY_STICKY_ENV */ +}; I would add a typedef for this struct like for the other structs here.
