From e26eff6902c6cb9ac5bc707115a3945215cbc8bc Mon Sep 17 00:00:00 2001 From: Nick Muerdter Date: Sun, 18 Jun 2017 08:03:24 -0600 Subject: [PATCH] Fix sockproc startup race conditions after running out of memory. If the `auto_ssl` shared dict ran out of memory, and then nginx were reloaded, then a race condition existed where multiple "sockproc" processes could try to be started at the same time. While I think this situation would be unlikely to affect a production system in a negative way (since the race condition only occurs during nginx reloads and 1 of the sockproc processes should still succeed and allow things to work), it did lead to some errors being logged, which would intermittently cause our tests to fail (it would crop up on the test following our t/memory.t test, since that next test would be the first reload following our test to explicitly exhaust the memory). This is fixed by using the `auto_ssl_settings` shared dict for storing the resty-lock details (the lock prevents multiple processes from being started at once). This smaller shared dict (introduced in #68) is used for storing bits of data that won't grow in size so we can better ensure the data will never be evicted from the cache. I'm now able to repeatedly run the test suite in a loop without hitting this edge case. Note that we are still using the `auto_ssl` shared dict for storing resty-lock details for domain registrations, since the memory requirements for that may grow (since there's a lock per domain, it's dynamic in size). But that should be okay, because similar to the SSL certs stored in `auto_ssl`, we're okay with cache evictions for old data in those cases (along with warnings being logged). Also possibly relevant is that currently resty-lock always uses `add` for the shared dict (so it will evict old data to make room if necessary), but there's a pull request for allowing use of `safe_add`: https://github.com/openresty/lua-resty-lock/pull/6 While we should be okay by switching things to `auto_ssl_settings` (since we should never have enough stored data in there to need evicting old data), it's something to keep an eye on. Related to: - https://github.com/GUI/lua-resty-auto-ssl/pull/68#issuecomment-305078619 - https://github.com/GUI/lua-resty-auto-ssl/issues/48#issuecomment-294397379 --- lib/resty/auto-ssl/jobs/renewal.lua | 2 +- lib/resty/auto-ssl/utils/shell_execute.lua | 1 + lib/resty/auto-ssl/utils/start_sockproc.lua | 8 +++++++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/resty/auto-ssl/jobs/renewal.lua b/lib/resty/auto-ssl/jobs/renewal.lua index a1c7c57..3347056 100644 --- a/lib/resty/auto-ssl/jobs/renewal.lua +++ b/lib/resty/auto-ssl/jobs/renewal.lua @@ -133,7 +133,7 @@ local function do_renew(auto_ssl_instance) if not get_interval_lock("renew", auto_ssl_instance:get("renew_check_interval")) then return end - local renew_lock, new_renew_lock_err = lock:new("auto_ssl", { exptime = 1800, timeout = 0 }) + local renew_lock, new_renew_lock_err = lock:new("auto_ssl_settings", { exptime = 1800, timeout = 0 }) if new_renew_lock_err then ngx.log(ngx.ERR, "auto-ssl: failed to create lock: ", new_renew_lock_err) return diff --git a/lib/resty/auto-ssl/utils/shell_execute.lua b/lib/resty/auto-ssl/utils/shell_execute.lua index f01cd12..408db3e 100644 --- a/lib/resty/auto-ssl/utils/shell_execute.lua +++ b/lib/resty/auto-ssl/utils/shell_execute.lua @@ -17,6 +17,7 @@ return function(command) wait_time = wait_time + sleep_time if wait_time > max_time then + ngx.log(ngx.ERR, "auto-ssl: sockproc did not start in expected amount of time") break end end diff --git a/lib/resty/auto-ssl/utils/start_sockproc.lua b/lib/resty/auto-ssl/utils/start_sockproc.lua index c8449a8..7b8e1ae 100644 --- a/lib/resty/auto-ssl/utils/start_sockproc.lua +++ b/lib/resty/auto-ssl/utils/start_sockproc.lua @@ -2,7 +2,13 @@ local auto_ssl = require "resty.auto-ssl" local lock = require "resty.lock" local function start() + local _, set_false_err = ngx.shared.auto_ssl_settings:safe_set("sockproc_started", false) + if set_false_err then + ngx.log(ngx.ERR, "auto-ssl: failed to set shdict for sockproc_started: ", set_false_err) + end + ngx.log(ngx.NOTICE, "auto-ssl: starting sockproc") + local exit_status = os.execute("umask 0022 && " .. auto_ssl.lua_root .. "/bin/resty-auto-ssl/start_sockproc") -- Lua 5.2+ returns boolean. Prior versions return status code. if exit_status == 0 or exit_status == true then @@ -21,7 +27,7 @@ return function(force) end -- Add lock to ensure only a single start command is attempted at a time. - local start_lock, new_lock_err = lock:new("auto_ssl", { exptime = 600, timeout = 0 }) + local start_lock, new_lock_err = lock:new("auto_ssl_settings", { exptime = 600, timeout = 0 }) if new_lock_err then ngx.log(ngx.ERR, "Failed to create lock: ", new_lock_err) return