flukso / lua-mosquitto

Lua bindings to the libmosquitto MQTT client library.
https://github.com/flukso/lua-mosquitto
Other
63 stars 42 forks source link

Callbacks are called from other threads #21

Closed daurnimator closed 6 years ago

daurnimator commented 6 years ago

When in async mode, callbacks seem to be called when mosquitto feels like it, which is entirely undefined behaviour.

local mqtt = require "mosquitto"
local gettid do
    local ffi = require "ffi" -- use luajit or https://github.com/facebookarchive/luaffifb
    ffi.cdef[[int syscall(int);]]
    gettid = function()
        return ffi.C.syscall(186)
    end
end
local client = mqtt.new()
client:callback_set("ON_CONNECT", function()
    print("connected", gettid())
    client:subscribe("$SYS/#")
end)
client:callback_set("ON_MESSAGE", function(mid, topic, payload)
    print("message", gettid(), mid, topic, payload)
end)
local broker = arg[1] or "localhost"
assert(client:connect_async(broker))
assert(client:loop_start())

client:loop_read()
client:loop_read()
client:loop_write()
client:loop_misc()
client:loop_read()
client:loop_read()
client:loop_write()
client:loop_misc()
client:loop_read()
client:loop_read()
client:loop_write()
karlp commented 6 years ago

your code here is completely wrong. loop_start starts a new thread. you then do not keep trying to call loop_read/write/misc yourself. the callback is called from the loop thread yes, this is well explained in the mosquitto reference

daurnimator commented 6 years ago

your code here is completely wrong. loop_start starts a new thread. you then do not keep trying to call loop_read/write/misc yourself. the callback is called from the loop thread yes, this is well explained in the mosquitto reference

Ah oops. I misread what :loop_start was for. Looks like I just call :loop_read/write/misc as required.

daurnimator commented 6 years ago

So it appears that :loop_start is always unsafe to use in the style that lua-mosquitto binds it. Could you remove it from the API?

karlp commented 6 years ago

You're perfectly free to implement your own locking and access controls, and there are totally uses where you don't need anything shared, where having it called from other threads is completely ok. I don't see any reason to just go and flat remove the binding, that seems overreaching.