Deep userdata system does not properly copy id_func

[dptr1988]

The id_func for deep userdata objects appears to be a normal lua_CFunction type function, but yet it is not copied or accessed properly if various places of the deep userdata system.

If the id_func depends on upvalues or on Lua's automatic stack cleanup, it would not work with the deep userdata system, since it takes a pointer to the function from L and then pushes a new closure onto L2 without copying upvalues. Also luaG_deep_userdata ( other places too? ) it just calls the id_func directly through the function pointer, rather than calling through the closure with lua_call.

It's not that hard to write your id_func to not rely on closures, module initialization and automatic stack cleanup, so simply mentioning these limitations in the documentation could be all that is needed to address this issue right now.

[benoit-germain]

The way I see it, luaG_deep_userdata receives the id_func on the stack only because of lanes.lua. If we remove mm.linda_id, and replace mm._deep_userdata with some function with no parameters that instanciates a linda, then this need goes away. lanes.linda() becomes a pure C closure, we can change the prototype of luaG_deep_userdata to accept the id_func pointer directly, and the id_func proto to accept a string argument. This way it becomes obvious that this is not a regular lua_CFunction, that shouldn't rely on upvalues and proper stack cleanup.
The only drawback is that there is an interface change for luaG_deep_userdata, which means that modules using the deep userdata system will have to rebuild.

Done (locally, since I don't have github access here; I'll create a branch so that you can have a look at the changes). The changes are pretty straightforward. Now all calls to idfunc are direct (no more lua_call), and the function pointer is stored as a light userdata to avoid calling it by error (and I can't push a lua_CFunction anyway since the proto changed).

[dptr1988]

This sounds like a good solution to the problem. It should help clarify the interface between C modules and lanes.

Does this change affect the issue where the id_func is called after the associated shared library has been unloaded? If id_func was a C closure, it could be registered ( with the Register cclosure feature ) which would make sure that the shared library was always loaded as long as the closure existed. But if id_func is now just a normal function pointer, how does Lanes make sure that the shared library associated with the id_func pointer remains loaded until the selfdestruct_atext() function is called ( which is the end of Lanes )?

[benoit-germain]

Unfortunately, since idfunc becomes a regular C function pointer, we can't force the shared library to remain loaded with the changes I have made so far.

My idea to fix this would be for the foo module writer to grab the userdata containing the library handle in the main state registry right inside its luaopen_foo() call (it can be found at "LOADLIB:foo"), and register it through a new luaG_ function provided by lanes that would clone it directly in the keeper state's registry (along with its metatable). This would mean that as long as the keeper state exists the shared library won't be unloaded though.

Edit: Another idea: state that id_func should answer a new query (say, "module") that pushes the name of the module that exports it on the stack. Inside luaG_push_proxy(), after metatable is cached, call id_func("module"), and require the module in the target Lua state.

[dptr1988]

This may be slightly off topic: Is there any way we can restructure the current Lanes to allow a single module to be usable without Lanes and with Lanes?

Right now, it appears like C modules using userdata must recompile a special version of the module to make it compatible with Lanes.

Ideally, an end user should be able to setup whatever is needed to make a module work with Lanes, but since this doesn't appear possible, the next best thing would be to make it easy for module writers to include Lanes support into their module without having to maintain a separate version or inconvenience themselves too much.

So if we are already going to be breaking backwards compatibility with previous versions of the deep UD system ( with this change you suggested ), we may want to consider redesigning the interface for a more flexible approach that will allow modules to selectively enable/disable Lanes support for their userdata at runtime.

[benoit-germain]

It is even worse than that: in order to be compatible, the module must link against lanes since it must push userdata on the stack with luaG_deep_userdata(). So even if someone uses the module in a single threaded application, the lanes shared library must be available...

[benoit-germain]

What about moving the keeper states shutdown in selfdestruct_atexit() just before the Lanes chained list shutdown sequence? Then at least as long as a Lane's Lua state that required the module is here, calling the id_func inside the keeper state shouldn't crash? Now, if the module was only registered in the main thread, I guess it was shut down before we enter selfdestruct_atexit(), so it is too late.

[dptr1988]

What about moving the keeper states shutdown in selfdestruct_atexit just before the lane chained list shutdown sequence?

...

This should solve the problem for all deep userdatas, except for the Linda deep UD, since they use keeper_acquire keeper_call in their __gc routine. But other than that, it should solve the problem since all of the other Lua states would have the option of requiring the appropriate module needed to run the deep UD __gc routine.

This would also mean that thread cleanup functions ( gc and finalizers ) would not be able to use Linda objects at all. I'm currently using Lindas in the set_finalizer call backs and although it wouldn't be affected by this ( since all my threads shutdown properly before the main state is closed ), I could easily see a use case where multiple threads would use Lindas to coordinate the shutdown process.

[dptr1988]

It is even worse than that: in order to be compatible, the module must ...

luaG_deep_userdata is already a Lua callable function, the id_func is more or less a Lua callable function, so that leaves luaG_todeep as the only reason why a module shared library needs to link to Lanes.

I'm not sure exactly why luaG_todeep is a plain C function, other than for performance reasons, since some of the other functions ( specifically the id_func interface ) already pass around pointers as lightuserdatas. So I don't think it would be too much of a stretch to change luaG_todeep to be Lua callable and return a lightuserdata instead of being a C function that returns a void *

This would allow module makers to provide lanes support at runtime on request ( like with an enable_lanes() function? ), optionally if package.loaded['lanes'] existed or unconditionally with require('lanes').

[benoit-germain]

So what you propose is that we expose C closures accessible only from C, stored for example in the main state registry, that perform the operations of luaG_push_deepuserdata() and luaG_todeep()? Then a module author, to be Lanes-compatible, should search for these functions, and elicit to either use them when available (Lanes was required), or to stick with the regular Lua API otherwise?

My feeling about all this is that we have two types of module authors:

• the first type develops his modules for internal use, he knows he will use them with Lanes (and no other multithreading solution), and he crafts them so that they work with Lanes from the start. There is no need for him to handle the case where Lanes is not installed on the system using his module.
• the second type develops a general purpose module released in the public domain, and there is no particular reason he should take care of Lanes compatibility, since Lanes is one solution among others to do Lua multithreading. IOW, I don't expect anybody to write public Lanes-compatible modules. Therefore, if someone wants to use any binary module and Lanes together, it is the module user's responsibility to use this module in conjunction with Lanes in a way that he can do what he needs (therefore not passing around userdata not intended for that purpose).

So, I believe that making all necessary functions lua_CFunctions so that they can be obtained as C closures once lanes is required will only complexify the work for the module writer that wants to create a Lanes-compatible module. Directly calling luaG_push_userdata() and luaG_todeep(), as drop-in replacement for lua_pushuserdata() and lua_touserdata() is much simpler (with the only additional step of writing the idfunc).

So, to my mind, keeping the API as it is (direct call just like any Lua API function) is the way to go. Unless you can convince me otherwise :-)

[dptr1988]

I guess I was getting a little too ambitious thinking that module authors would consider adding Lanes support to their publicly available modules. After reading your description of the two types of module authors, I agree with you on that. Making the Lanes API functions directly callable ( like the Lua API ) is definitely much cleaner/clearer, so for the lack of good reasons to use any other method, this probably is the best way to do it.

Thank you for taking the time to discuss the idea.

[dptr1988]

Edit: Another idea: state that id_func should answer a new query (say, "module") that pushes the name of the module that exports ...

...

After playing around with various options available to solve this issue, having Lanes automatically require the appropriate module in the new state seemed to be the best solution. And getting the module name from the id_func is about as simple as you can make it, so IMO, this idea you came up with is an excellent solution to the deep UD id_func shared library dependency problem.

[benoit-germain]

I've submitted the necessary changes. Can we consider this closed?

[dptr1988]

Yes, looks like it's all fixed now. Thank you making these changes to fix this!