'bad request' in connect when using lua-resty-memcached

jon.keys · 2014-09-03T00:16:08+00:00

Hello! On Tue, Sep 2, 2014 at 3:18 AM, ashish adhav wrote: > I meant light threads. > Yes, there is plan to add a builtin pool for the Lua thread obj...

'bad request' in connect when using lua-resty-memcached

jon.keys

I have a nonce caching function setup in an init_worker_by_lua_file:

local ngx = require "ngx"
local myapp = require "myapp"
local memcached = require "resty.memcached"

-----------------------------------
-- Configuration
-----------------------------------
local nonce_client_timeout = 1000 -- 1 sec
local nonce_exptime = 300 -- 5 min
local memc_nonce_servers = {
    {
        host = "192.168.1.100",
        port = 11211
    },
    {
        host = "192.168.1.101",
        port = 11211
    }
}


local idle_conn_timeout = 10000
local max_pool_size = 100
-----------------------------------

local memc_nonce = nil
local has_init = false

local function initClient()
    has_init = true
    local inst_nonce_err = nil
    memc_nonce, inst_nonce_err = memcached:new()
    if not memc_nonce then
        memc_nonce = nil
        ngx.log(ngx.ERR, "Unable to create memcached nonce instance: ", (inst_nonce_err or "unkown error"))
        return
    end
    memc_nonce:set_timeout(nonce_client_timeout)
end

local function connectNonce()
    if not has_init then initClient() end
    if memc_nonce == nil then return false end
    for i = 1, #memc_nonce_servers do
        local memc_nonce_server = memc_nonce_servers[i]
        local nonce_ok, nonce_conn_err = memc_nonce:connect(memc_nonce_server.host, memc_nonce_server.port)
        if not nonce_ok then
            ngx.log(ngx.ERR, "Unable to connect to memcached nonce server " .. memc_nonce_server.host .. ": " .. (nonce_conn_err or "unkown error"))
        else
            return true
        end
    end
    return false
end

local function closeNonceConnection(no_keepalive)
    if no_keepalive then
        memc_nonce:close()
        return
    end
    local ok, close_nonce_err = memc_nonce:set_keepalive(idle_conn_timeout, max_pool_size)
    if not ok then
        memc_nonce:close()
        ngx.log(ngx.ERR, "Unable to set memcached nonce connection keepalive: ", (close_nonce_err or "unkown error"))
    end
end

local function cacheNonce(key)
    if not connectNonce() then
        -- do not block a potentially valid request because of memcached outages
        return true
    end
    local valid_nonce = true
    local ok, nonce_err = memc_nonce:add(key, 1, nonce_exptime)
    if not ok then
        if nonce_err == "NOT_STORED" then
            valid_nonce = false
            nonce_err = nil
        else
            ngx.log(ngx.ERR, "Unable to add memcached nonce value: ", (nonce_err or "unkown error"))
        end
    end
    closeNonceConnection(nonce_err)
    return valid_nonce
end

local nonce_fn = function(acct_id, nonce_val)
    if acct_id == nil  or nonce_val == nil then
        return "missing account ID or nonce value", false
    end
    if not cacheNonce(acct_id .. nonce_val) then
        return "duplicate nonce", false
    end
    return nil, true
end
myapp.setNonceFn(nonce_fn)

The 'myapp' module uses the nonce function if it is present:

local _M = { _VERSION = '0.01' }

local nonce_fn = nil

function _M.setNonceFn(nonce_lookup_fn)
    nonce_fn = nonce_lookup_fn
end

function _M.check_nonce(acct_id, nonce_val)
    if nonce_fn == nil then
        return true
    end
    return nonce_fn(acct_id, nonce_val)
end

return _M

I don't get any errors on the first request.

On the second request with a duplicate nonce I do get an error:

2014/09/02 15:36:21 [error] 21134#0: *23 lua entry thread aborted: runtime error: app/init_worker.lua:87: bad request
stack traceback:
coroutine 0:

[C]: in function 'connect'

/usr/local/openresty/nginx/app/init_worker.lua:46: in function 'connectNonce'

/usr/local/openresty/nginx/app/init_worker.lua:69: in function 'cacheNonce'

/usr/local/openresty/nginx/app/init_worker.lua:91: in function 'nonce_fn'

What is really strange is this is only happening in my test environment and not my dev environment.

I'm using the latest stable OpenResty 1.7.2.1 in both dev and test environments.

Any advice would be greatly appreciated.

agentzh

Hello!

On Tue, Sep 2, 2014 at 9:16 AM, Jon Keys wrote:
>> 2014/09/02 15:36:21 [error] 21134#0: *23 lua entry thread aborted: runtime
>> error: app/init_worker.lua:87: bad request

https://github.com/openresty/lua-resty-memcached#limitations

"The resty.memcached object instance cannot be stored in a Lua
variable at the Lua module level, because it will then be shared by
all the concurrent requests handled by the same nginx worker process
(see http://wiki.nginx.org/HttpLuaModule#Data_Sharing_within_an_Nginx_Worker
) and result in bad race conditions when concurrent requests are
trying to use the sameresty.memcached instance. You should always
initiate resty.memcached objects in function local variables or in the
ngx.ctx table. These places all have their own data copies for each
request."

https://github.com/openresty/lua-nginx-module#ngxsockettcp

"The cosocket object created by this API function (ngx.socket.tcp) has
exactly the same lifetime as the Lua handler creating it. So never
pass the cosocket object to any other Lua handler (including ngx.timer
callback functions) and never share the cosocket object between
different NGINX requests."

In summary: do not share lua-resty-memcached objects among your
requests; recycle your underlying memcached connections via the
connection pools (see the "set_keepalive" method).

Regards,
-agentzh

jon.keys

On Tuesday, September 2, 2014 1:06:50 PM UTC-4, agentzh wrote:

Hello!

On Tue, Sep 2, 2014 at 9:16 AM, Jon Keys wrote:
>> 2014/09/02 15:36:21 [error] 21134#0: *23 lua entry thread aborted: runtime
>> error: app/init_worker.lua:87: bad request

https://github.com/openresty/lua-resty-memcached#limitations

"The resty.memcached object instance cannot be stored in a Lua
variable at the Lua module level, because it will then be shared by
all the concurrent requests handled by the same nginx worker process
(see http://wiki.nginx.org/HttpLuaModule#Data_Sharing_within_an_Nginx_Worker
) and result in bad race conditions when concurrent requests are
trying to use the sameresty.memcached instance. You should always
initiate resty.memcached objects in function local variables or in the
ngx.ctx table. These places all have their own data copies for each
request."

https://github.com/openresty/lua-nginx-module#ngxsockettcp

"The cosocket object created by this API function (ngx.socket.tcp) has
exactly the same lifetime as the Lua handler creating it. So never
pass the cosocket object to any other Lua handler (including ngx.timer
callback functions) and never share the cosocket object between
different NGINX requests."

In summary: do not share lua-resty-memcached objects among your
requests; recycle your underlying memcached connections via the
connection pools (see the "set_keepalive" method).

OK, got it. So this means I need to create a new memcached object ( e.g. 'memcached:new()' ) for every request?

I am already using 'set_keepalive' but I didn't realize I needed to throw away the memcached object after each request.

Apologies if I am being obtuse.

Regards,
-agentzh

agentzh

Hello!

On Tue, Sep 2, 2014 at 10:11 AM, Jon Keys wrote:
>
> OK, got it. So this means I need to create a new memcached object ( e.g.
> 'memcached:new()' ) for every request?

Yes. Allocating the Lua table in the new() call is usually WAY cheaper
than the subsequent socket syscalls. If you're sensitive to
performance then I suggest you avoid doing memcached communication
upon every request by means of caching (in case of fetches) or batch
processing (in case of writes).

Are you seeing this new() call being hot in the on-CPU flame graphs
for your real-world setup?

> I am already using 'set_keepalive' but I didn't realize I needed to throw
> away the memcached object after each request.
>

Throwing away the memcached object is WAY cheaper than throwing away
the underlying connection.

This share-nothing model on the Lua land simplifies a lot of things in
concurrent processing and can avoid a lot of unneeded race conditions
:)

Let's optimize things according to real-world profiling results (like
flame graphs) :)

Regards,
-agentzh

jon.keys

On Tuesday, September 2, 2014 2:09:01 PM UTC-4, agentzh wrote:

Hello!

On Tue, Sep 2, 2014 at 10:11 AM, Jon Keys wrote:
>
> OK, got it. So this means I need to create a new memcached object ( e.g.
> 'memcached:new()' ) for every request?

Yes. Allocating the Lua table in the new() call is usually WAY cheaper
than the subsequent socket syscalls. If you're sensitive to
performance then I suggest you avoid doing memcached communication
upon every request by means of caching (in case of fetches) or batch
processing (in case of writes).

Understood.

Are you seeing this new() call being hot in the on-CPU flame graphs
for your real-world setup?

No no. Nothing like that.

I was just trying to be clever and re-use the object. Shot myself in the foot on that one :-)

> I am already using 'set_keepalive' but I didn't realize I needed to throw
> away the memcached object after each request.
>

Throwing away the memcached object is WAY cheaper than throwing away
the underlying connection.

This share-nothing model on the Lua land simplifies a lot of things in
concurrent processing and can avoid a lot of unneeded race conditions
:)

Absolutely. I was thinking that each worker would get its own memcached object and it would be safe since the workers are single-threaded.

Again, just me trying to be clever when I should have kept it simple.

Let's optimize things according to real-world profiling results (like
flame graphs) :)

Totally agree. Thanks so much for your help! Once again you've saved the day :-)

Regards,
-agentzh

agentzh

Hello!

On Tue, Sep 2, 2014 at 11:18 AM, Jon Keys wrote:
> I was thinking that each worker would get its own memcached
> object and it would be safe since the workers are single-threaded.
> Again, just me trying to be clever when I should have kept it simple.
>

Well, certain types of race conditions can still happen among multiple
concurrent "light threads" during I/O time even when only a single OS
thread is involved.

It is still *very* likely that multiple downstream requests are trying
to operate on same cosocket object at the same time. For example,
nginx request handler A is trying to send a new memcached request
while nginx request handler B is already waiting for its own memcached
response on the same memcached connection. In such cases, you'll
surely have an access conflict and an inconsistent context.

Regards,
-agentzh

jon.keys

On Tuesday, September 2, 2014 2:31:42 PM UTC-4, agentzh wrote:

Hello!

On Tue, Sep 2, 2014 at 11:18 AM, Jon Keys wrote:
> I was thinking that each worker would get its own memcached
> object and it would be safe since the workers are single-threaded.
> Again, just me trying to be clever when I should have kept it simple.
>

Well, certain types of race conditions can still happen among multiple
concurrent "light threads" during I/O time even when only a single OS
thread is involved.

It is still *very* likely that multiple downstream requests are trying
to operate on same cosocket object at the same time. For example,
nginx request handler A is trying to send a new memcached request
while nginx request handler B is already waiting for its own memcached
response on the same memcached connection. In such cases, you'll
surely have an access conflict and an inconsistent context.

I see. Thanks for the explanation.

As a general rule of thumb is it always better to create a new object per request or is it just for those objects which do async I/O?

For example, if the object is not doing any I/O (e.g. just calculating hash, encoding base64, etc...) do I still need to create a new object per request?

In my current app I do a lot of signature verification and so I keep an HMAC object for each worker and just reset the internal HMAC context for each request. I chose not to create a new object because creating a new HMAC context for each request likely would be very expensive.

Regards,
-agentzh

agentzh

Hello!

On Tue, Sep 2, 2014 at 11:44 AM, Jon Keys wrote:
> As a general rule of thumb is it always better to create a new object per
> request or is it just for those objects which do async I/O?
> For example, if the object is not doing any I/O (e.g. just calculating hash,
> encoding base64, etc...) do I still need to create a new object per request?

You can share changeable data among all the concurrent requests as
long as there is NO nonblocking I/O operations (including ngx.sleep)
in the middle of your calculations. As long as you do not give the
control back to the nginx event loop and ngx_lua's light thread
scheduler (even implicitly), there can never be any race conditions in
between. So always be careful when you share changeable data on the
worker level. Buggy optimizations can easily lead to hard-to-debug
race conditions under load.

BTW, most of the (nonblocking) IO operations on the Lua land (like
cosocket operations) are actually *synchronous*. Nonblocking I/O is a
much better term here. The term "async I/O" can also easily get
confused with the kernel AIO feature.

Regards,
-agentzh

jon.keys

On Tuesday, September 2, 2014 2:55:25 PM UTC-4, agentzh wrote:

Hello!

On Tue, Sep 2, 2014 at 11:44 AM, Jon Keys wrote:
> As a general rule of thumb is it always better to create a new object per
> request or is it just for those objects which do async I/O?
> For example, if the object is not doing any I/O (e.g. just calculating hash,
> encoding base64, etc...) do I still need to create a new object per request?

You can share changeable data among all the concurrent requests as
long as there is NO nonblocking I/O operations (including ngx.sleep)
in the middle of your calculations. As long as you do not give the
control back to the nginx event loop and ngx_lua's light thread
scheduler (even implicitly), there can never be any race conditions in
between. So always be careful when you share changeable data on the
worker level. Buggy optimizations can easily lead to hard-to-debug
race conditions under load.

BTW, most of the (nonblocking) IO operations on the Lua land (like
cosocket operations) are actually *synchronous*. Nonblocking I/O is a
much better term here. The term "async I/O" can also easily get
confused with the kernel AIO feature.

OK, got it. Thanks so much for the detailed explanation. It is really helpful.

Regards,
-agentzh