openresty / lua-resty-redis

Lua redis client driver for the ngx_lua based on the cosocket API
1.9k stars 448 forks source link

failed to set keepalive: connection in dubious state #78

Open hcaihao opened 8 years ago

hcaihao commented 8 years ago

local redis = require "resty.redis" local red = redis:new()

red:set_timeout(1000) -- 1 sec

local ok, err = red:connect("127.0.0.1", 6379) if not ok then ngx.say("failed to connect: ", err) return end

--ngx.say("connected to redis.")

local key = "testkey" red:set(key, false)

local ok, err = red:set_keepalive(10000, 100) if not ok then ngx.say("failed to set keepalive: ", err) return end

The code bellow throw an error: "failed to set keepalive: connection in dubious state"

I know redis set support string value only, But "red:set(key, true)" will be ok, why?

agentzh commented 8 years ago

@hcaihao You should always check the return values of the set() method call and handle any errors since this call can fail.

hcaihao commented 8 years ago

The problem is what's the difference between "red:set(key, true)" and "red:set(key, false)"?

doujiang24 commented 8 years ago

@hcaihao

  1. redis only have string type in kv, so you'd better to use red:set(key, "false") instead
  2. false is not valid in resty.redis now, so redis return err: "ERR Protocol error: invalid bulk length" and send tcp close frame immediately, then the sock is now readable, this is why you got "connection in dubious state".

@agentzh May be we can just raise an error here? Seems we don't need to support this in send. Or tostring first? https://github.com/openresty/lua-resty-redis/blob/master/lib/resty/redis.lua#L260

agentzh commented 8 years ago

@hcaihao This is because the Lua false value maps to the redis NULL value on the protocol level, which is not supported by the redis set command at all and thus results in an error returned. That's why you should always handle any errors properly yourself.

@doujiang24 I'm not sure about that. We could raise an exception for this particular case but I still think people should always check return values of the redis queries anyway.

doujiang24 commented 8 years ago

@agentzh Total agree people should always check return values. But I think the error ERR Protocol error: invalid bulk length is a higher level ERROR in redis, so redis close the connection. Other errors like ERR wrong number of arguments for 'set' command, redis won't close the connection.

So, maybe just always tostring first can be acceptable? https://github.com/openresty/lua-resty-redis/pull/79

hcaihao commented 8 years ago

@agentzh @doujiang24 Thk for your explanation! I see it.

agentzh commented 8 years ago

@doujiang24 Yeah, I'm fine with tostring when the input argument is not a string.