[policy] run __gc metamethod when policies are garbage collected #688
Conversation
gateway/src/apicast/policy.lua
Outdated
| end | ||
|
|
||
| local function metatable(policy) | ||
| local proxy = newproxy(true) |
There was a problem hiding this comment.
To be honest, I learnt about newproxy while reviewing this PR.
If I understood it correctly we need to do this to include __gc in the table. It looks like this is not needed in Lua 5.2+ and will not be needed in future versions of LuaJIT: LuaJIT/LuaJIT#47
I think this merits a comment.
There was a problem hiding this comment.
It definitely needs plenty of comments.
I seriously doubt it will land in LuaJIT because it depends on LuaJIT/LuaJIT#38. Even though LuaJIT/LuaJIT#47 is closed it is not implemented.
Another approach is to use void pointer from ffi and use ffi.gc like:
local ffi = require('ffi')
local p = ffi.new('void *')
ffi.gc(p, function() print('__gc') end)
collectgarbage()
p = nil
collectgarbage()
| @@ -219,6 +219,8 @@ function lazy.rewrite(configuration, host) | |||
| ngx.log(ngx.WARN, 'failed to acquire lock to lazy load: ', host, ' error: ', err) | |||
| end | |||
|
|
|||
| collectgarbage() | |||
There was a problem hiding this comment.
In some configurations when reloading, this line is not executed. I think we need to call collectgarbage also in refresh_configuration, or somewhere else that runs when re-loading in 'boot' mode.
There was a problem hiding this comment.
Pushed a commit that fixes this.
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| local cjson = require('cjson') | ||
|
|
||
| local tonumber = tonumber | ||
| local new = _M.new | ||
|
|
||
| function mt.__gc(policy) |
gateway/src/apicast/policy.lua
Outdated
| return setmetatable({}, mt) | ||
| local p = setmetatable({}, mt) | ||
|
|
||
| p.gc = GC.delegate(p) |
There was a problem hiding this comment.
They have both pointer to the other, so their life is tied together.
When the gc object is GC'd it will call the __gc method on the policy metatable.
Using the userdata as proxy had significant performance issue in my early benchmarks, but can't reproduce it now, so they might have been invalid.
There are several other issues with the delegation on policies: calling next or type. Those are used by inspect and by some our code as well. We would have to patch next and type methods to rely on some other metamethods.
I agree this is less than ideal and will explore the approach of allocating metatable per policy instance, so it can be GCd with the policy.
gateway/src/apicast/module.lua
Outdated
| @@ -39,7 +39,7 @@ local name = env.value('APICAST_MODULE') or 'apicast.policy.apicast' | |||
| local ok, mod = prequire(name) | |||
|
|
|||
| if ok and mod then | |||
| if type(mod) == 'table' then | |||
| if type(mod) == 'table' or true then | |||
There was a problem hiding this comment.
I know. Needed to do this temporarily because type(mod) returns userdata.
gateway/src/apicast/policy.lua
Outdated
| @@ -18,7 +18,8 @@ local PHASES = { | |||
| 'ssl_certificate', | |||
| } | |||
|
|
|||
| local setmetatable = setmetatable | |||
| local GC = require('apicast.gc') | |||
| local setmetatable = GC.setmetatable | |||
There was a problem hiding this comment.
Maybe we should rename this var to avoid confusion.
When I read this diff for the first time I assumed that we were using the normal setmetatable function.
gateway/src/apicast/gc.lua
Outdated
| __gc = function(self) | ||
| local mt = getmetatable(self.table) | ||
|
|
||
| if mt and mt.__gc then return mt.__gc(table) end |
There was a problem hiding this comment.
This should be self.table.
* use just 1 core * pin to a specific CPU core
spec/policy_spec.lua
Outdated
| mt.__gc = spy.new(function(p) property = p.someproperty end) | ||
| local policy = MyPolicy.new() | ||
| policy.someproperty = 'foobar' | ||
| policy = nil |
There was a problem hiding this comment.
value assigned to variable 'policy' is unused
spec/policy_spec.lua
Outdated
| local MyPolicy, mt = policy.new('my_policy', '1.0') | ||
| local property | ||
| mt.__gc = spy.new(function(p) property = p.someproperty end) | ||
| local policy = MyPolicy.new() |
There was a problem hiding this comment.
shadowing upvalue 'policy' on line 1
| @@ -3,7 +3,7 @@ | |||
| -- Also can interrupt the execution and skip the current phase or | |||
| -- the whole processing of the request. | |||
|
|
|||
| local _M = require('apicast.policy').new('Echo Policy') | |||
| local _M, mt = require('apicast.policy').new('Echo Policy') | |||
mikz
changed the title
|
@davidor sorry for rebasing your commits, but I think it is better when the relevant changes (code, tests) are in one commit and I needed to make a change anyway to cover the |
|
@mikz No worries, I agree with that approach. |
When policy is reloaded it is GC'd and gets __gc metamethod called. That can be used for example for unregistering timers.
This would allow doing cleanup when policies are reloaded by changed configuration (like shutting down timers).