Anaminus / roblox-bug-tracker

Formerly an unofficial bug tracker for Roblox.
33 stars 24 forks source link

BindableFunctions and BindableEvents clone tables #740

Closed cntkillme closed 8 years ago

cntkillme commented 8 years ago

It makes no sense to have tables cloned considering the table already exists on the same machine and most people probably don't want to clone the table in the first place. For whatever reason the tables are probably being serialized when invoking/firing the bindable. This is confusing, a waste of memory, and the metatable is lost.

Reproduction:

local bf = Instance.new("BindableFunction");
bf.OnInvoke = function(tbl) print("BF", tbl, getmetatable(tbl)); end;

local test = setmetatable({}, {});
print("Test", test, getmetatable(test));
bf:Invoke(test);

Output:

Test table: (addr of table) table: (addr of metatable)
BF table: (addr of another table) nil
chc4 commented 8 years ago

That's because BindableFunctions use the same machinary as RemoteFunctions, which /have/ to serialize the table to send over the network. I'd imagine it's a feature that if it works with a BindableFunction, it works with a Remote one.

Another potential problem is that priviledged scripts, such as CoreScripts or StarterScript, run in a seperate Lua state and can register BindableFunction callbacks for normal scripts to use. In order to transfer the table over it would need unsafe pointer cloning which is potentially buggy, instead of just pushing and popping on the state.

This is pretty much the same thing with #739, too. You can't serialize a metatable, userdata or otherwise, nicely 99% of the time, unless your __index metamethod is just a lookup table or you start getting fancy and serialize the functions down to bytecode and back (which has a problem where you now can't capture upvalues for closures)

cntkillme commented 8 years ago

Yeah I figured, back to _G yeyyyyyy except I find it stupid that it is being serialized at all. It shouldn't have to be sent over anything.

feathrs commented 8 years ago

So what's being said here is that because they use the same code as RemoteEvents, this is another muckup that Roblox won't fix even though it shouldn't have needed fixing if it was done right in the first place?

Anaminus commented 8 years ago

Bindables were implemented the way they were in anticipation that one day they would operate across a network. This has since been implement as Remotes. Bindables retain their original behavior so that they don't break the expectation that values are copied. This distinction is important, and given the widespread use of Bindables, cannot be changed.

feathrs commented 8 years ago

Somebody is going to hang for this.

AnonymousDapper commented 8 years ago

Wow. That's some bs right there