lqt icon indicating copy to clipboard operation
lqt copied to clipboard

lqt is not coroutine-safe; objects created in coroutines other than the one that loaded lqt misbehave

Open ToxicFrog opened this issue 12 years ago • 6 comments

Specifically, you can create objects in threads other than the loader, but you cannot then connect their signals to anything, or strange error messages from QCoreApplication::exec or QCoreApplication::processEvents (depending on which one your mainloop is using) will result.

Note that you can still call methods on the object from other threads, and there's no requirement that app.exec() be called from the same thread that created the object - but if the object itself is not created in the same thread that called require "qtcore", sadness occurs.

Here's a test case:

require "qtcore"
require "qtgui"

local app = QApplication(1, arg)

local window

coroutine.resume(coroutine.create(function()
    window = QPushButton("click me")
end))

window:connect("2clicked()", print)
window:show()
app.exec()
--
-- output: QApplication::exec(QPushButton*): incorrect or extra arguments, expecting: .

The expected output is a bunch of "userdata" from the connected print().

Rearranging the code so that the requires of qtcore are in a separate coroutine and the creation of the QPushButton is in main results in a different but no less odd error message:

coroutine.resume(coroutine.create(function()
    require "qtcore"
    require "qtgui"
end))

local app = QApplication(1, arg)

local window
window = QPushButton("click me")

window:connect("2clicked()", print)
window:show()

app.exec()
--
-- output: attempt to call a nil value

Moving everything into main, or everything into the same coroutine, works fine. (I don't know what happens if you require "qtcore" from one thread and "qtgui" from a different one.)

Based on this behaviour, I suspect that when loaded, lqt is remembering the lua_State * that loaded it and is sometimes interacting with that stack even when it should be interacting with the stack that it was called from, but honestly I'm not even sure where to start debugging this, let alone writing a patch.

ToxicFrog avatar Jan 23 '12 04:01 ToxicFrog

I ran into this same issue, as I'm trying to use QT in heavily threaded (coroutines) code. I can see a little bit of why this is occurring - the lua_State variable used when connecting signals is obtained from the lua_State that is used when require the modules. I'm interested in working on this but to make a patch will require spending substantially more time understanding the lqt framework.

dmattp avatar Jun 15 '12 15:06 dmattp

The current framework has its limits. I am not sure lqt was designed with coroutines in mind. The fact is that each object with virtual methods keeps the state it was created in and then uses it. I see no way how to handle multiple Lua states (coroutines), so any ideas would be welcome.

mkottman avatar Jun 15 '12 20:06 mkottman

I think things are pretty close to working, the only trouble I've run into so far is in running slot callbacks in the coroutine. The only thing necessary is that the lua_State variable is preserved from the "connect" call to the callback from LqtSlotAcceptor::__slot(...). The limitation as far as I can tell is that the LqtSlotAcceptor objects are created in eg, qtgui_meta.cpp / luaopen_qtgui, and we create a single (?) slot acceptor object with the lua_State "L" that is passed to luaopen_qtgui. When "connect" is called from a coroutine, the coroutine has a different lua_State pointer, and that shows up properly in [common/lqt_qt.cpp - lqtL_connect]- but when the signal gets invoked, the slot acceptor has the original "L" pointer used to open the library, not the "L" pointer that was seen in the lqtL_connect call. For the signal/slot callbacks to work with multiple coroutines, it seems like what needs to happen is that the lua_State variable provided to the lqtL_connect() call is what needs to be used by the LqtSlotAcceptor::__slot(...) function.

I don't have a good enough understanding of the overall framework to know how difficult that change would be. It might mean creating a new LqtSlotAcceptor object for each connect call, or maybe just having one for each coroutine and selecting the proper one. I may get back to looking at it, as being able to use lqt across multiple coroutines would be very powerful! But for the most part I can get by doing all my QT stuff in the main lua thread, and it's not clear to me yet how the :connect() / lqtL_connect mechanism ends up attaching QT's slots/signal to the LqtSlotAcceptor.

dmattp avatar Jun 15 '12 20:06 dmattp

The framework is relatively simple once you get the overview. The base is that every class which contains virtual methods gets a "shell" class, which allows you to override the methods in Lua.

Among other things, the object also "registers" itself into the Lua state it was created in. Also, there is one generic "slot acceptor" for each module (ie lqtSlotAcceptor_qtcore) which handles all known slot signatures (this also means you cannot create new slot signatures). This acceptor is created when the module is loaded and saves the Lua state in which it was created.

One way to make this more useful could be, as you have hinted, to create one slot acceptor for each object when the object is created. Then it would be a little bit more usable, but still you would be bound to the thread in which you created the object. For reference, here is the generated code for accepting the metacall:

int lqt_shell_QObject::qt_metacall(QMetaObject::Call call, int index, void **args) {
        index = QObject::qt_metacall(call, index, args);
        if (index < 0) return index;
        return lqtL_qt_metacall(L, this, lqtSlotAcceptor_qtcore, call, "QObject*", index, args);
}

Here, L is the saved pointer in the shell object instance:

class LQT_EXPORT lqt_shell_QObject : public QObject {
public:
  lua_State *L;
  lqt_shell_QObject (lua_State *l, QObjectPrivate& arg1, QObject* arg2) : QObject(arg1, arg2), L(l) {
    lqtL_register(L, this);
  }
  lqt_shell_QObject (lua_State *l, QObject* arg1) : QObject(arg1), L(l) {
    lqtL_register(L, this);
  }
  ~lqt_shell_QObject() { lqtL_unregister(L, this); }

Maybe one way to solve this problem would be to allow an object to be "moved to thread", i.e.:

void moveToThread(lua_State *L) { this->L = L; }

And then bind this method to Lua, so that in your Lua code, you could do this:

local object = QObject()
local thread
thread = coroutine.create(function()
    object:moveToThread(thread)
    -- further processing
end)

This would change the L saved in the object and hopefully make the whole coroutine-business work :)

mkottman avatar Jun 16 '12 06:06 mkottman

FWIW, I'm revisiting this again, and it looks to me like the easiest way to handle this is to just always set the objects' lua_State to the main thread. As long as the lqt libraries are required in the main thread (not in a coroutine) then the signal callbacks seem to work fine even with objects that have been created in a coroutine. In Lua 5.2 the main thread can be found like this:

lua_State* lqtL_getmainthread (lua_State *L) {
   lua_rawgeti(L, LUA_REGISTRYINDEX, LUA_RIDX_MAINTHREAD);
   lua_State* main = lua_tothread(L,-1);
   lua_pop(L,1);
   return main;
}

(There may be an easier way to get the main thread's state, but this was the first thing that occurred to me). So I added that function in lqt_common.cpp/.hpp, and changed the generator in virtuals.lua/fill_shell_class() to look like this:

        cline = cline .. ') : ' .. c.xarg.fullname
                  .. '(' .. argline .. [[), L(lqtL_getmainthread(l)) {
                       lqtL_register(L, this);
                  ]]

So that the object's lua_State is always initialized to the main thread's state rather than the coroutine's State.

dmattp avatar Nov 07 '14 16:11 dmattp

I suspect your suggestion about moving the objects to thread would work also, e.g., if you really wanted to require the lqt libraries from a coroutine, but for me there is no reason not to just make everything point to the main thread, and it's simpler than moving individual objects to different threads each time it's needed.

dmattp avatar Nov 07 '14 16:11 dmattp