Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround to make __gc work on tables #790

Merged
merged 3 commits into from
Jun 26, 2018
Merged

Workaround to make __gc work on tables #790

merged 3 commits into from
Jun 26, 2018

Conversation

davidor
Copy link
Contributor

@davidor davidor commented Jun 26, 2018

In LuaJIT and Lua 5.1, the __gc metamethod does not work with tables, it only works with "userdata". This PR introduces a module that implements a workaround to make it work with tables.

@davidor davidor requested a review from a team as a code owner June 26, 2018 09:53
spec/gc_spec.lua Outdated
setmetatable(table, { __gc = function() end })
stub(getmetatable(table), '__gc')

local table_with_gc = GC.enable(table)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable 'table_with_gc' is never accessed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK. We need to define it so it's garbage collected later.

Copy link
Contributor Author

@davidor davidor Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approved this one manually in codeclimate.

@davidor davidor force-pushed the gc-for-tables branch 2 times, most recently from d8ab645 to 996cd84 Compare June 26, 2018 10:57
@3scale 3scale deleted a comment from codeclimate bot Jun 26, 2018

mt.__index = table
mt.__newindex = table
mt.__table = table
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice to also hide the this metatable by:

mt.__metatable = getmetatable(table)

Then getmetatable(proxy) will return the metatable returned by getmetatable(table).

This would not propagate if someone would change the metatable of table directly, but that can be avoided by changing the API a bit.

end
end

function _M.enable(table)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could rename this library to setmetatable_gc(table, mt).

It could make more sense when using the __metatable key.
All changes to GC'd objects metatables should be done with this method.

mt.__newindex = table
mt.__table = table

mt.__len = function() return #table end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid allocating functions for each object.

These can be shared by (even when using __metatable):

local rawgetmetatable = debug.getmetatable
local getmetatable = getmetatable

local function __len(t)
  return #(rawgetmetatable(t).__table)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about debug.getmetatable 👍

end

local function __call(proxy, ...)
local t = getmetatable(proxy).__table
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should defined this as:

getmetatable(proxy).__table(...)

Because now it basically defines all tables callable.

Unfortunately that can yield rather cryptic error message: attempt to call field '__table' (a nil value).

Unfortunately Lua tries to be helpful and the error message depends on how you call the table:

  • attempt to call local 'varname' (a table value)
  • attempt to call a table value

So replicating those messages might not be possible.

Based on how the function is stored (or if it is a field).
The only workaround I can imagine is using pcall like:

local function __call(self, ...)
  local mt = rawgetmetatable(self)
  local ret = { pcall(mt.__table, ...) }

  local ok = table.remove(ret, 1)

  if ok then
	  return unpack(ret)
  else
	  error(ret[1], 2)
  end
end

That would throw error: ERROR: gc.lua:66: attempt to call a table value (even with correct line pointing to the original caller).

spec/gc_spec.lua Outdated
stub(getmetatable(test_table), '__gc')

local table_with_gc = GC.enable(test_table)
table_with_gc = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could avoid that CC error by assert(table_with_gc) right ?

spec/gc_spec.lua Outdated

local table_with_gc = GC.set_metatable_gc(test_table, test_metatable)
assert(table_with_gc)
table_with_gc = nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value assigned to variable 'table_with_gc' is unused

return pairs(original_table(proxy))
end

function _M.set_metatable_gc(table, metatable)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shadowing upvalue 'table' on line 11

@davidor davidor force-pushed the gc-for-tables branch 3 times, most recently from 510c797 to 4dd556e Compare June 26, 2018 14:02
@davidor
Copy link
Contributor Author

davidor commented Jun 26, 2018

@mikz I addressed your comments. In some cases, I adopted a slightly different solution. Can you take a look?

end

local function __tostring(proxy)
local mt = getmetatable(proxy)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not calling tostring(original_table(proxy)) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was needed because I wasn't calling setmetatable(t, metatable) at the beginning of set_metatable_gc. I needed to do that, otherwise, __index would not work.
I fixed this in a new commit. Please check it. I'll squash it after review.

Copy link
Contributor

@mikz mikz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This is great!

@davidor davidor merged commit ea79660 into master Jun 26, 2018
@davidor davidor deleted the gc-for-tables branch June 26, 2018 16:03
@davidor davidor mentioned this pull request Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants