Giter Club home page Giter Club logo

Comments (13)

agentzh avatar agentzh commented on July 29, 2024 2

@freman I've noticed one obvious mistake that you do not declare the local variables red (and also err) in your function _M.new(self), which means the red variable is actually a global variable that is still shared by concurrent requests.

It is recommended to always use the lua-releng tool to scan all your Lua code for such accidental use of Lua globals where you really want locals. See https://github.com/openresty/lua-nginx-module#lua-variable-scope

from lua-resty-redis.

agentzh avatar agentzh commented on July 29, 2024

@freman ngx_lua's cosocket objects can only be used by the request creating them while you're sharing the cosocket object in your module table across all the requests, thus leading to the "bad request" exception. See https://github.com/openresty/lua-resty-redis#limitations

The correct way is to create separate cosocket objects (hence separate resty.redis instances) for each request handler.

from lua-resty-redis.

freman avatar freman commented on July 29, 2024

I thought that's what I was doing by initing the module for every request.

from lua-resty-redis.

agentzh avatar agentzh commented on July 29, 2024

@freman Lua modules are cached and shared globally unless you turn off lua_code_cache (turning off the code cache is not recommended to production use but just for quick development). See https://github.com/openresty/lua-nginx-module#data-sharing-within-an-nginx-worker

from lua-resty-redis.

agentzh avatar agentzh commented on July 29, 2024

@freman The correct way is to make your module initiate new objects for individual requests in an OO-style way, just like lua-resty-redis itself. That way, the states can be cleanly separated and prevent unwanted state sharing.

from lua-resty-redis.

freman avatar freman commented on July 29, 2024

Here I was hoping to reduce the code duplication, now I'm on my way down the rabbit hole :D

from lua-resty-redis.

freman avatar freman commented on July 29, 2024

I think I'm still messing something up @agentzh - still newish at lua, this would be my first object orientated script (and previously my first module) - I pretty much resorted to copying what you had done.

local _M = {}
local mt = { __index = _M }

local REDIS_TIMEOUT   = 250
local REDIS_HOST      = '127.0.0.1'
local REDIS_PORT      = 6379
local REDIS_POOLSIZE  = 200
local REDIS_IDLETIME  = 60
local MAP_EXPIRES     = 3660

function _M.new(self)
    local redis = require "resty.redis"
    red = redis:new()
    err = false

    red:set_timeout(REDIS_TIMEOUT)

    local ok, err = red:connect(REDIS_HOST, REDIS_PORT)
    if not ok then
        ngx.log(ngx.ERR, "Absolute failure of redis: " .. err)
        return nil
    end

    return setmetatable({ red = red, err = err }, mt)
end

function _M.fetch(self)
    local res, err = self.red:get(ngx.var.uid_got)
    if not res then
        ngx.log(ngx.ERR, "Failed to get value for key (" .. ngx.var.uid_got .. "): " .. err)
        self.err = true
        return
    end

    if type(res) == "string" then
        ngx.var.httphost = res
        local res, err = self.red:expire(ngx.var.uid_got, MAP_EXPIRES)
        if not res then
            ngx.log(ngx.ERR, "Failed to reset expiry for (" .. ngx.var.uid_got ..") to " .. MAP_EXPIRES .. ": " .. err)
            self.err = true
            return
        end
    end
    return true
end

function _M.store(self)
    local res, err = self.red:setex(ngx.var.uid_got, MAP_EXPIRES, ngx.var.httphost)
    if not res then
        ngx.log(ngx.ERR, "Failed to store value (" .. ngx.var.httphost .. ") for key (" .. ngx.var.uid_got .. "): " .. err)
        self.err = true
        return
    end

--  if ngx.var.args then
--      ngx.log(ngx.ERR, "Not redirecting...")
--      return true
--  end

    return "/" .. ngx.var.redirecturl
end

function _M.clear(self)
    local res, err = self.red:del(ngx.var.uid_got)
    if not res then
        ngx.log(ngx.ERR, "Failed to clear value for key (" .. ngx.var.uid_got .. "): " .. err)
        self.err = true
        return
    end
end

function _M.pool(self)
    if self.err then
        ngx.log(ngx.ERR, "Not sending closed connection back to pool")
    else
        self.red:set_keepalive(REDIS_IDLETIME, REDIS_POOLSIZE)
    end
end

return _M

It's still spitting a few bad request errors out of redis.lua:292

2014/05/12 04:17:32 [error] 71#0: *31 lua entry thread aborted: runtime error: /usr/share/lua/5.1/resty/redis.lua:292: bad request
stack traceback:
coroutine 0:
    [C]: in function 'send'
    /usr/share/lua/5.1/resty/redis.lua:292: in function 'get'
    /etc/nginx/conf.d/lua/hostmap.lua:28: in function 'fetch'
    /etc/nginx/conf.d/lua/select_host.lua:5: in function </etc/nginx/conf.d/lua/select_host.lua:1>, client: 172.17.42.1, server: www.example.com, request: "GET /css/odds_and_rules.css HTTP/1.1", host: "www.yoda.lan", referrer: "https://www.yoda.lan/"
2014/05/12 04:17:32 [error] 71#0: *32 lua entry thread aborted: runtime error: /usr/share/lua/5.1/resty/redis.lua:292: bad request
stack traceback:
coroutine 0:
    [C]: in function 'send'
    /usr/share/lua/5.1/resty/redis.lua:292: in function 'get'
    /etc/nginx/conf.d/lua/hostmap.lua:28: in function 'fetch'
    /etc/nginx/conf.d/lua/select_host.lua:5: in function </etc/nginx/conf.d/lua/select_host.lua:1>, client: 172.17.42.1, server: www.example.com, request: "GET /css/index.css HTTP/1.1", host: "www.yoda.lan", referrer: "https://www.yoda.lan/"
2014/05/12 04:17:32 [error] 71#0: *30 lua entry thread aborted: runtime error: /usr/share/lua/5.1/resty/redis.lua:292: bad request
stack traceback:
coroutine 0:
    [C]: in function 'send'
    /usr/share/lua/5.1/resty/redis.lua:292: in function 'get'
    /etc/nginx/conf.d/lua/hostmap.lua:28: in function 'fetch'
    /etc/nginx/conf.d/lua/select_host.lua:5: in function </etc/nginx/conf.d/lua/select_host.lua:1>, client: 172.17.42.1, server: www.example.com, request: "GET /js/global.js HTTP/1.1", host: "www.yoda.lan", referrer: "https://www.yoda.lan/"

And select_host.lua

if ngx.var.uid_got then
    local hostmap = require "hostmap"
    if hostmap then
        local map = hostmap:new()
        if map:fetch() then
            map:pool()
        end
    end
end

from lua-resty-redis.

agentzh avatar agentzh commented on July 29, 2024

@freman BTW, it's also better to move the line local redis = require "resty.redis" to the toplevel of your Lua module scope because it does no good to introduce require() calls into your hot Lua code paths.

from lua-resty-redis.

freman avatar freman commented on July 29, 2024

Thank you very much for your help and advice @agentzh, I believe it's working now without fault

from lua-resty-redis.

agentzh avatar agentzh commented on July 29, 2024

@freman Good to know :) I'm closing this.

from lua-resty-redis.

morksinaanab avatar morksinaanab commented on July 29, 2024

Hi @agentzh I seem to have this same bad request error when trying to get or set, and I can't figure out what to improve. I was also trying to use pcall to be able to serve an alternative if there is a bad request. I don't see where I would not have local. I thought it is. What am I not getting?

local imageserver = {}

local originalDir
local resizedDir
local testDir
local utils
local defaultWidth
local defaultHeight
local defaultQuality
local redis
local red
local magick
local ok, err

function imageserver.init()

    utils = require "utils"
    defaultWidth = 450
    defaultHeight = 450
    defaultQuality = 70
    magick = require "magick"
    redis = require "redis"


    testDir = '/original/'
    originalDir = '/mount/'
    resizedDir = '/resized/'

    imageserver.init_redis()

end

function imageserver.done()

    -- put it into the connection pool of size 100,
    -- with 10 seconds max idle time
    ok, err = red:set_keepalive(5000, 2000)
    if ok then
        utils.log("redis","set keepalive success")
    else
        utils.log("redis","failed to set keepalive: ", err)
    end

    -- or just close the connection right away:
--   ok, err = red:close()
--   if not ok then
--       utils.log("failed to close: ", err)
--   end

    return

end

function imageserver.resize_and_compress(image,width,height,quality)
    local source = originalDir .. image
    local key = imageserver.key_for_image(image,width,height,quality)
    local destination = resizedDir .. key
    local size 

    if not utils.file_exists(source) or not utils.file_is_valid(source) then
        return utils.return_404()
    end

    if not width and not height then
        width = defaultWidth
        height = defaultHeight
    end
    if not width then
        width = ""
    end
    if not height then
        height = ""
    end
    if not quality then 
        quality = defaultQuality
    end

    size = width .. "x" .. height



    magick.thumb(source, size, destination)

    return destination
end

function imageserver.put_in_cache(key,value)
    ok = false


    ok, err = red:set(key, value)
    if ok then
        utils.log("redis","set " .. key)
    else
        utils.log("redis","failed to set " .. key .. " : " .. err)
    end

    return ok
end

function imageserver.init_redis()
    timeout = 200;


    red = redis:new()
    red:set_timeout(timeout)

    ok, err = red:connect("127.0.0.1", 6379)
    if ok then
        utils.log("redis","success connecting to redis")
    else
        utils.log("redis","failed to connect: " .. err)
    end

    return

end

function imageserver.get_from_cache(key)
    -- return value matching key -- 


    utils.log("redis","the connection reused:" ..   red:get_reused_times())
    if not red then
        utils.log("redis","red not here?")
        return nil  
    else 

         result, errr = red:get(key)
        if not result then
            utils.log("redis", "failed to get " .. key .. " : " .. errr)
            return nil
        end

        if result == ngx.null then
            utils.log("redis","no data for " .. key)
            return nil
        end

    end


    return result
end

-- Cache Handling --
function imageserver.clear_cache(key)
    if key == nil then
        -- clear all cache --
    else
        imageserver.put_in_cache(key,nil)
    end
end

function imageserver.key_for_image_original(image)
    return image
end

function imageserver.key_for_image(image,width,height,quality)
    local file, ext = image:match("([^.]+).([^.]+)")
    local key = string.gsub(file,"/","_")

    if not key then
        -- return nil
    elseif width == nil and height == nil and quality == nil then
        key = key .. "_0_0_0"
    elseif width == nil and quality == nil then
        key = key .. "_0_" .. height .. "_0"
    elseif height == nil and quality == nil then
        key = key .. "_" .. width .. "_0_0"
    elseif width == nil and height == nil then
        key = key .. "_0_0_" .. quality
    elseif width == nil and quality == nil then
        key = key .. "_0_" .. height .. "_0"
    elseif width == nil then
        key = key .. "_0_" .. height .. "_" .. quality
    elseif height == nil then
        key = key .. "_" .. width .. "_0_" .. quality
    elseif quality == nil then
        key = key .. "_" .. width .. "_" .. height .. "_0"
    else
        key = key .. "_" .. width .. "_" .. height .. "_" .. quality    
    end

    if not not key then
        key = key .. "." .. ext
    end

    return key;
end

function imageserver.parse_request(request,arguments)
    parameters = {}

    parameters['image'] = request
    parameters['width'] = tonumber(arguments['w'])
    parameters['height'] = tonumber(arguments['h'])
    parameters['quality'] = tonumber(arguments['q'])
    parameters['original'] = tonumber(arguments['o'])

    return parameters

end

function imageserver.serve(image,width,height,quality)

    local imageData
    local key 

    key = imageserver.key_for_image(image,width,height,quality) 
    imageData = imageserver.get_from_cache(key)

    if not key then
        return utils.return_404()
    elseif not imageData then
        local filename = imageserver.resize_and_compress(image,width,height,quality)

        imageData = imageserver.get_from_file(filename)

        imageserver.put_in_cache(key,imageData)
        imageserver.output_image(imageData)
    else
        imageserver.output_image(imageData)
    end 

end

function imageserver.serve_original(image)
    local imageData = imageserver.get_from_file(originalDir .. image)

    if not imageData then
        utils.return_404()
    else
        imageserver.output_image(imageData)
    end

end

function imageserver.output_image(imageData)
    --[[
    utils.output(imageData)
    --]]
    ---[[
     utils.output_image(imageData) 
    --]]

end

function imageserver.get_from_file(filename)

    local data
    local fileHandle
    fileHandle = io.open (filename, 'rb')

    if not fileHandle then
      return nil
    else 
        imageData = fileHandle:read('*a')
        return imageData
    end

end


function imageserver.validate_parameters(parameters)

    if not parameters['image'] then
        -- we need the image at least --
        return false
    elseif parameters['original'] and utils.table_count(parameters) > 2 then
        -- if original is asked, we don't allow any other parameters --
        return false
    elseif not parameters['original'] and utils.table_count(parameters) > 4 then
        -- if no original asked, but somehow there are more than 4 params, that's weird --
        return false
    elseif parameters['quality'] and (not utils.is_int(parameters['quality']) or parameters['quality'] > 100 or parameters['quality'] < 1 ) then
        -- quality needs to be int between 0-100 --
        return false
    elseif parameters['width'] and ( not utils.is_int(parameters['width']) or parameters['width'] > 2550 or parameters['width'] < 1 )then
        -- width needs to be int between 1-2550 --
        return false
    elseif parameters['height'] and ( not utils.is_int(parameters['height']) or parameters['height'] > 2550 or parameters['height'] < 1 ) then
        -- height needs to be int between 1-2550 --
        return false
    else
        return true
    end
end

function imageserver.serve_from_request(request,arguments)
    parameters = imageserver.parse_request(request,arguments)

    if imageserver.validate_parameters(parameters) then

        if utils.starts(parameters['image'],'test/') then
            parameters['image'] = string.sub(parameters['image'],6)
            originalDir = testDir
        end

        utils.log("serve",originalDir)
        if not parameters['original']  then
            imageserver.serve(parameters['image'],parameters['width'],parameters['height'],parameters['quality'])
        else    
            imageserver.serve_original(parameters['image'])
        end

    else 
        utils.return_404()
    end

end



return imageserver

from lua-resty-redis.

agentzh avatar agentzh commented on July 29, 2024

@morksinaanab As I've said earlier, this is a clear sign of misusing the cosocket API. Please read my previous comments in this ticket for more details. BTW, please do not paste lengthy examples since I will not have the time to read it. Always try to provide a minimal and standalone example that is easy to digest for others if you want to provide any. Thanks for your cooperation!

from lua-resty-redis.

taogogo avatar taogogo commented on July 29, 2024

i have jump into this bad hole,and thanks veriy much.

from lua-resty-redis.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.