Re: Just me? or is rewrite broke?

[Brian Pane]

William A. Rowe, Jr. wrote:

>mod_rewrite.c:182
>    /* the cache */
>static cache *cachep;
>
>looks very, very evil on a threaded architecture.
>

It definitely looks broken.  Here's a patch to add a lock...
not the most scalable approach, but it's the quickest solution
I can think of.

--Brian



Index: modules/mappers/mod_rewrite.h
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/mappers/mod_rewrite.h,v
retrieving revision 1.25
diff -u -r1.25 mod_rewrite.h
--- modules/mappers/mod_rewrite.h       2001/05/18 18:38:42     1.25
+++ modules/mappers/mod_rewrite.h       2001/10/20 23:40:26
@@ -112,6 +112,9 @@
 #include <sys/types.h>
 #endif
 
+#if APR_HAS_THREADS
+#include "apr_lock.h"
+#endif
 #include "ap_config.h"
 
     /* Include from the Apache server ... */
@@ -322,6 +325,9 @@
 typedef struct cache {
     apr_pool_t         *pool;
     apr_array_header_t *lists;
+#if APR_HAS_THREADS
+    apr_lock_t *lock;
+#endif
 } cache;
 
 
Index: modules/mappers/mod_rewrite.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/mappers/mod_rewrite.c,v
retrieving revision 1.82
diff -u -r1.82 mod_rewrite.c
--- modules/mappers/mod_rewrite.c       2001/08/15 21:11:58     1.82
+++ modules/mappers/mod_rewrite.c       2001/10/20 23:40:28
@@ -3665,6 +3665,9 @@
     if (apr_pool_create(&c->pool, p) != APR_SUCCESS)
                return NULL;
     c->lists = apr_array_make(c->pool, 2, sizeof(cachelist));
+#if APR_HAS_THREADS
+    (void)apr_lock_create(&(c->lock), APR_MUTEX, APR_INTRAPROCESS, NULL, p);
+#endif
     return c;
 }
 
@@ -3755,6 +3758,10 @@
     cachetlbentry *t;
     int found_list;
 
+#if APR_HAS_THREADS
+    apr_lock_acquire(c->lock);
+#endif
+
     found_list = 0;
     /* first try to edit an existing entry */
     for (i = 0; i < c->lists->nelts; i++) {
@@ -3767,6 +3774,9 @@
             if (e != NULL) {
                 e->time  = ce->time;
                 e->value = apr_pstrdup(c->pool, ce->value);
+#if APR_HAS_THREADS
+                apr_lock_release(c->lock);
+#endif
                 return;
             }
 
@@ -3775,8 +3785,11 @@
                 if (strcmp(e->key, ce->key) == 0) {
                     e->time  = ce->time;
                     e->value = apr_pstrdup(c->pool, ce->value);
-                  cache_tlb_replace((cachetlbentry *)l->tlb->elts,
-                                    (cacheentry *)l->entries->elts, e);
+                    cache_tlb_replace((cachetlbentry *)l->tlb->elts,
+                                      (cacheentry *)l->entries->elts, e);
+#if APR_HAS_THREADS
+                    apr_lock_release(c->lock);
+#endif
                     return;
                 }
             }
@@ -3807,11 +3820,17 @@
             e->value = apr_pstrdup(c->pool, ce->value);
             cache_tlb_replace((cachetlbentry *)l->tlb->elts,
                               (cacheentry *)l->entries->elts, e);
+#if APR_HAS_THREADS
+            apr_lock_release(c->lock);
+#endif
             return;
         }
     }
 
     /* not reached, but when it is no problem... */
+#if APR_HAS_THREADS
+    apr_lock_release(c->lock);
+#endif
     return;
 }
 
@@ -3822,23 +3841,37 @@
     cachelist *l;
     cacheentry *e;
 
+#if APR_HAS_THREADS
+    apr_lock_acquire(c->lock);
+#endif
+
     for (i = 0; i < c->lists->nelts; i++) {
         l = &(((cachelist *)c->lists->elts)[i]);
         if (strcmp(l->resource, res) == 0) {
 
             e = cache_tlb_lookup((cachetlbentry *)l->tlb->elts,
                                  (cacheentry *)l->entries->elts, key);
-            if (e != NULL)
+            if (e != NULL) {
+#if APR_HAS_THREADS
+                apr_lock_release(c->lock);
+#endif
                 return e;
+            }
 
             for (j = 0; j < l->entries->nelts; j++) {
                 e = &(((cacheentry *)l->entries->elts)[j]);
                 if (strcmp(e->key, key) == 0) {
+#if APR_HAS_THREADS
+                    apr_lock_release(c->lock);
+#endif
                     return e;
                 }
             }
         }
     }
+#if APR_HAS_THREADS
+    apr_lock_release(c->lock);
+#endif
     return NULL;
 }