several fixes:

*) do not add event if file was used less than min_uses
*) do not rely upon event to avoid race conditions
*) ngx_open_file_lookup()

1 files changed, 239 insertions, 165 deletions

diff --git a/src/core/ngx_open_file_cache.c b/src/core/ngx_open_file_cache.c
index 5748f9e68..9c5cc6732 100644
--- a/src/core/ngx_open_file_cache.c
+++ b/src/core/ngx_open_file_cache.c
@@ -18,15 +18,21 @@
 
 
 static void ngx_open_file_cache_cleanup(void *data);
+static ngx_int_t ngx_open_and_stat_file(u_char *name, ngx_open_file_info_t *of,
+    ngx_log_t *log);
+static void ngx_open_file_add_event(ngx_open_file_cache_t *cache,
+    ngx_cached_open_file_t *file, ngx_open_file_info_t *of, ngx_log_t *log);
 static void ngx_open_file_cleanup(void *data);
 static void ngx_close_cached_file(ngx_open_file_cache_t *cache,
     ngx_cached_open_file_t *file, ngx_uint_t min_uses, ngx_log_t *log);
-static ngx_int_t ngx_open_and_stat_file(u_char *name, ngx_open_file_info_t *of,
-    ngx_log_t *log);
+static void ngx_open_file_del_event(ngx_cached_open_file_t *file);
 static void ngx_expire_old_cached_files(ngx_open_file_cache_t *cache,
     ngx_uint_t n, ngx_log_t *log);
 static void ngx_open_file_cache_rbtree_insert_value(ngx_rbtree_node_t *temp,
     ngx_rbtree_node_t *node, ngx_rbtree_node_t *sentinel);
+static ngx_cached_open_file_t *
+    ngx_open_file_lookup(ngx_open_file_cache_t *cache, ngx_str_t *name,
+    uint32_t hash);
 static void ngx_open_file_cache_remove(ngx_event_t *ev);
 
 
@@ -124,11 +130,9 @@ ngx_open_cached_file(ngx_open_file_cache_t *cache, ngx_str_t *name,
     time_t                          now;
     uint32_t                        hash;
     ngx_int_t                       rc;
-    ngx_rbtree_node_t              *node, *sentinel;
     ngx_pool_cleanup_t             *cln;
     ngx_cached_open_file_t         *file;
     ngx_pool_cleanup_file_t        *clnf;
-    ngx_open_file_cache_event_t    *fev;
     ngx_open_file_cache_cleanup_t  *ofcln;
 
     of->err = 0;
@@ -159,163 +163,147 @@ ngx_open_cached_file(ngx_open_file_cache_t *cache, ngx_str_t *name,
         return NGX_ERROR;
     }
 
-    hash = ngx_crc32_long(name->data, name->len);
-
-    node = cache->rbtree.root;
-    sentinel = cache->rbtree.sentinel;
-
     now = ngx_time();
 
-    while (node != sentinel) {
+    hash = ngx_crc32_long(name->data, name->len);
 
-        if (hash < node->key) {
-            node = node->left;
-            continue;
-        }
+    file = ngx_open_file_lookup(cache, name, hash);
 
-        if (hash > node->key) {
-            node = node->right;
-            continue;
-        }
+    if (file) {
 
-        /* hash == node->key */
+        file->uses++;
 
-        do {
-            file = (ngx_cached_open_file_t *) node;
-
-            rc = ngx_strcmp(name->data, file->name);
-
-            if (rc == 0) {
+        ngx_queue_remove(&file->queue);
 
-                file->uses++;
+        if (file->fd == NGX_INVALID_FILE && file->err == 0 && !file->is_dir) {
 
-                ngx_queue_remove(&file->queue);
+            /* file was not used often enough to keep open */
 
-                if (file->fd == NGX_INVALID_FILE
-                    && file->err == 0
-                    && !file->is_dir)
-                {
-                    /* file was not used often enough to be open */
+            rc = ngx_open_and_stat_file(name->data, of, pool->log);
 
-                    rc = ngx_open_and_stat_file(name->data, of, pool->log);
+            if (rc != NGX_OK && (of->err == 0 || !of->errors)) {
+                goto failed;
+            }
 
-                    if (rc != NGX_OK && (of->err == 0 || !of->errors)) {
-                        goto failed;
-                    }
+            goto add_event;
+        }
 
-                    goto update;
-                }
+        if ((file->event && file->use_event)
+            || (file->event == NULL && now - file->created < of->valid))
+        {
+            if (file->err == 0) {
 
-                if (file->event || now - file->created < of->valid) {
+                of->fd = file->fd;
+                of->uniq = file->uniq;
+                of->mtime = file->mtime;
+                of->size = file->size;
 
-                    if (file->err == 0) {
+                of->is_dir = file->is_dir;
+                of->is_file = file->is_file;
+                of->is_link = file->is_link;
+                of->is_exec = file->is_exec;
 
-                        of->fd = file->fd;
-                        of->uniq = file->uniq;
-                        of->mtime = file->mtime;
-                        of->size = file->size;
+                if (!file->is_dir) {
+                    file->count++;
+                    ngx_open_file_add_event(cache, file, of, pool->log);
+                }
 
-                        of->is_dir = file->is_dir;
-                        of->is_file = file->is_file;
-                        of->is_link = file->is_link;
-                        of->is_exec = file->is_exec;
+            } else {
+                of->err = file->err;
+            }
 
-                        if (!file->is_dir) {
-                            file->count++;
-                        }
+            goto found;
+        }
 
-                    } else {
-                        of->err = file->err;
-                    }
+        ngx_log_debug4(NGX_LOG_DEBUG_CORE, pool->log, 0,
+                       "retest open file: %s, fd:%d, c:%d, e:%d",
+                       file->name, file->fd, file->count, file->err);
 
-                    goto found;
-                }
+        if (file->is_dir) {
 
-                ngx_log_debug4(NGX_LOG_DEBUG_CORE, pool->log, 0,
-                               "retest open file: %s, fd:%d, c:%d, e:%d",
-                               file->name, file->fd, file->count, file->err);
+            /*
+             * chances that directory became file are very small
+             * so test_dir flag allows to use a single syscall
+             * in ngx_file_info() instead of three syscalls
+             */
 
-                if (file->is_dir) {
+            of->test_dir = 1;
+        }
 
-                    /*
-                     * chances that directory became file are very small
-                     * so test_dir flag allows to use a single ngx_file_info()
-                     * syscall instead of three syscalls
-                     */
+        rc = ngx_open_and_stat_file(name->data, of, pool->log);
 
-                    of->test_dir = 1;
-                }
+        if (rc != NGX_OK && (of->err == 0 || !of->errors)) {
+            goto failed;
+        }
 
-                rc = ngx_open_and_stat_file(name->data, of, pool->log);
+        if (of->is_dir) {
 
-                if (rc != NGX_OK && (of->err == 0 || !of->errors)) {
-                    goto failed;
-                }
+            if (file->is_dir || file->err) {
+                goto update;
+            }
 
-                if (of->is_dir) {
-                    if (file->is_dir || file->err) {
-                        goto update;
-                    }
+            /* file became directory */
 
-                    /* file became directory */
+        } else if (of->err == 0) {  /* file */
 
-                } else if (of->err == 0) {  /* file */
+            if (file->is_dir || file->err) {
+                goto add_event;
+            }
 
-                    if (file->is_dir || file->err) {
-                        goto update;
-                    }
+            if (of->uniq == file->uniq
+                && of->mtime == file->mtime
+                && of->size == file->size)
+            {
+                if (ngx_close_file(of->fd) == NGX_FILE_ERROR) {
+                    ngx_log_error(NGX_LOG_ALERT, pool->log, ngx_errno,
+                                  ngx_close_file_n " \"%s\" failed",
+                                  name->data);
+                }
 
-                    if (of->uniq == file->uniq
-                        && of->mtime == file->mtime
-                        && of->size == file->size)
-                    {
-                        if (ngx_close_file(of->fd) == NGX_FILE_ERROR) {
-                            ngx_log_error(NGX_LOG_ALERT, pool->log, ngx_errno,
-                                          ngx_close_file_n " \"%s\" failed",
-                                          name->data);
-                        }
+                of->fd = file->fd;
+                file->count++;
 
-                        of->fd = file->fd;
-                        file->count++;
+                if (file->event) {
+                    file->use_event = 1;
+                    goto renew;
+                }
 
-                        goto renew;
-                    }
+                ngx_open_file_add_event(cache, file, of, pool->log);
 
-                    /* file was changed */
+                goto renew;
+            }
 
-                } else { /* error to cache */
+            /* file was changed */
 
-                    if (file->err || file->is_dir) {
-                        goto update;
-                    }
+        } else { /* error to cache */
 
-                    /* file was removed, etc. */
-                }
+            if (file->err || file->is_dir) {
+                goto update;
+            }
 
-                if (file->count == 0) {
-                    if (ngx_close_file(file->fd) == NGX_FILE_ERROR) {
-                        ngx_log_error(NGX_LOG_ALERT, pool->log, ngx_errno,
-                                      ngx_close_file_n " \"%s\" failed",
-                                      name->data);
-                    }
+            /* file was removed, etc. */
+        }
 
-                    goto update;
-                }
+        if (file->count == 0) {
 
-                ngx_rbtree_delete(&cache->rbtree, &file->node);
+            ngx_open_file_del_event(file);
 
-                cache->current--;
+            if (ngx_close_file(file->fd) == NGX_FILE_ERROR) {
+                ngx_log_error(NGX_LOG_ALERT, pool->log, ngx_errno,
+                              ngx_close_file_n " \"%s\" failed",
+                              name->data);
+            }
 
-                file->close = 1;
+            goto add_event;
+        }
 
-                goto create;
-            }
+        ngx_rbtree_delete(&cache->rbtree, &file->node);
 
-            node = (rc < 0) ? node->left : node->right;
+        cache->current--;
 
-        } while (node != sentinel && hash == node->key);
+        file->close = 1;
 
-        break;
+        goto create;
     }
 
     /* not found */
@@ -356,51 +344,16 @@ create:
 
     cache->current++;
 
-    file->count = 0;
     file->uses = 1;
+    file->count = 0;
+    file->use_event = 0;
+    file->event = NULL;
 
-update:
-
-    if (of->events
-        && (ngx_event_flags & NGX_USE_VNODE_EVENT)
-        && of->fd != NGX_INVALID_FILE)
-    {
-        file->event = ngx_calloc(sizeof(ngx_event_t), pool->log);
-        if (file->event== NULL) {
-            goto failed;
-        }
-
-        fev = ngx_alloc(sizeof(ngx_open_file_cache_event_t), pool->log);
-        if (fev == NULL) {
-            goto failed;
-        }
-
-        fev->fd = of->fd;
-        fev->file = file;
-        fev->cache = cache;
-
-        file->event->handler = ngx_open_file_cache_remove;
-        file->event->data = fev;
-
-        /*
-         * although vnode event may be called while ngx_cycle->poll
-         * destruction; however, cleanup procedures are run before any
-         * memory freeing and events will be canceled.
-         */
+add_event:
 
-        file->event->log = ngx_cycle->log;
+    ngx_open_file_add_event(cache, file, of, pool->log);
 
-        if (ngx_add_event(file->event, NGX_VNODE_EVENT, NGX_ONESHOT_EVENT)
-            != NGX_OK)
-        {
-            ngx_free(file->event->data);
-            ngx_free(file->event);
-            goto failed;
-        }
-
-    } else {
-        file->event = NULL;
-    }
+update:
 
     file->fd = of->fd;
     file->err = of->err;
@@ -550,6 +503,72 @@ ngx_open_and_stat_file(u_char *name, ngx_open_file_info_t *of, ngx_log_t *log)
 }
 
 
+/*
+ * we ignore any possible event setting error and
+ * fallback to usual periodic file retests
+ */
+
+static void
+ngx_open_file_add_event(ngx_open_file_cache_t *cache,
+    ngx_cached_open_file_t *file, ngx_open_file_info_t *of, ngx_log_t *log)
+{
+    ngx_open_file_cache_event_t  *fev;
+
+    if (!(ngx_event_flags & NGX_USE_VNODE_EVENT)
+        || !of->events
+        || file->event
+        || of->fd == NGX_INVALID_FILE
+        || file->uses < of->min_uses)
+    {
+        return;
+    }
+
+    file->event = ngx_calloc(sizeof(ngx_event_t), log);
+    if (file->event== NULL) {
+        return;
+    }
+
+    fev = ngx_alloc(sizeof(ngx_open_file_cache_event_t), log);
+    if (fev == NULL) {
+        ngx_free(file->event);
+        file->event = NULL;
+        return;
+    }
+
+    fev->fd = of->fd;
+    fev->file = file;
+    fev->cache = cache;
+
+    file->event->handler = ngx_open_file_cache_remove;
+    file->event->data = fev;
+
+    /*
+     * although vnode event may be called while ngx_cycle->poll
+     * destruction, however, cleanup procedures are run before any
+     * memory freeing and events will be canceled.
+     */
+
+    file->event->log = ngx_cycle->log;
+
+    if (ngx_add_event(file->event, NGX_VNODE_EVENT, NGX_ONESHOT_EVENT)
+        != NGX_OK)
+    {
+        ngx_free(file->event->data);
+        ngx_free(file->event);
+        file->event = NULL;
+        return;
+    }
+
+    /*
+     * we do not file->use_event here because there may be a race
+     * condition between opening file and adding event, so we rely
+     * upon event notification only after first file revalidation
+     */
+
+    return;
+}
+
+
 static void
 ngx_open_file_cleanup(void *data)
 {
@@ -585,14 +604,7 @@ ngx_close_cached_file(ngx_open_file_cache_t *cache,
         }
     }
 
-    if (file->event) {
-        (void) ngx_del_event(file->event, NGX_VNODE_EVENT,
-                             file->count ? NGX_FLUSH_EVENT : NGX_CLOSE_EVENT);
-
-        ngx_free(file->event->data);
-        ngx_free(file->event);
-        file->event = NULL;
-    }
+    ngx_open_file_del_event(file);
 
     if (file->count) {
         return;
@@ -618,6 +630,23 @@ ngx_close_cached_file(ngx_open_file_cache_t *cache,
 
 
 static void
+ngx_open_file_del_event(ngx_cached_open_file_t *file)
+{
+    if (file->event == NULL) {
+        return;
+    }
+
+    (void) ngx_del_event(file->event, NGX_VNODE_EVENT,
+                         file->count ? NGX_FLUSH_EVENT : NGX_CLOSE_EVENT);
+
+    ngx_free(file->event->data);
+    ngx_free(file->event);
+    file->event = NULL;
+    file->use_event = 0;
+}
+
+
+static void
 ngx_expire_old_cached_files(ngx_open_file_cache_t *cache, ngx_uint_t n,
     ngx_log_t *log)
 {
@@ -709,6 +738,51 @@ ngx_open_file_cache_rbtree_insert_value(ngx_rbtree_node_t *temp,
 }
 
 
+static ngx_cached_open_file_t *
+ngx_open_file_lookup(ngx_open_file_cache_t *cache, ngx_str_t *name,
+    uint32_t hash)
+{
+    ngx_int_t                rc;
+    ngx_rbtree_node_t       *node, *sentinel;
+    ngx_cached_open_file_t  *file;
+
+    node = cache->rbtree.root;
+    sentinel = cache->rbtree.sentinel;
+
+    while (node != sentinel) {
+
+        if (hash < node->key) {
+            node = node->left;
+            continue;
+        }
+
+        if (hash > node->key) {
+            node = node->right;
+            continue;
+        }
+
+        /* hash == node->key */
+
+        do {
+            file = (ngx_cached_open_file_t *) node;
+
+            rc = ngx_strcmp(name->data, file->name);
+
+            if (rc == 0) {
+                return file;
+            }
+
+            node = (rc < 0) ? node->left : node->right;
+
+        } while (node != sentinel && hash == node->key);
+
+        break;
+    }
+
+    return NULL;
+}
+
+
 static void
 ngx_open_file_cache_remove(ngx_event_t *ev)
 {