xHasKx / luamqtt

luamqtt - Pure-lua MQTT v3.1.1 and v5.0 client
https://xhaskx.github.io/luamqtt/
MIT License
154 stars 41 forks source link

refactor(loop) separate client and loop #31

Open Tieske opened 2 years ago

Tieske commented 2 years ago

So here's a PR with a LOT of changes, more than I had anticipated, and definitely breaking. I'll try to give an overview by functionality.

ioloop

The ioloop "runs" the process. It would read on all clients and run some functions, in a loop. Each read operation would however have a small, but fixed, timeout. This measn that if you have 20 clients, that a single iteration would always take minimum 20 * 0.005 seconds, (0.1 second). If one device is busy, and the others are idle, that would slow down the busy one.

Writes and connecting would always be a blocking operation. Meaning;

Changes: a client iteration now returns the maximum amount of time the client want's to sleep. The same for any functions added. The actual sleep is then the minimum of all those reported "maximums". Th read will be on timeout 0, so non-blocking at all. If a single client responds with "requested delay = 0" then the loop would not sleep, but immediately run the next iteration. Clients and functions can also report they are idle, in which case the wait will slowly increase with each idle iteration until a specified maximum. Once new data is received, (client no longer idle) the delay returns to 0. Writing and connecting is still blocking, albeit connecting now uses a proper timeout.

connector signals

The client code had hardcoded checks for some error messages, and didn't check all of them. Since the only code that can interpret the actual error messages is the connector itself, since it was written for that implementation (luascoket, ssl/luasec, copas or openresty). So instead 2 "signals" are now defined, first being "closed", when a read fails because the connection is closed by the server. The second "idle" indicating no data was available to be read, so the ioloop can increase the sleep. The connectors now implement the transform from their error messages to the common signals.

accommodating yielding and blocking sockets

The client should be agnostic to the different sockets types, yielding ones (copas and openresty, which are schedulers, so instead of blocking they'll go of and run other threads, until the read is complete), or the blocking sockets (luasocket and luasec/ssl, which simply block the ioloop while they process socket io). In either case the idea is to prevent a busy-loop while accommodating a responsive client. With the yielding ones the scheduler creates independent threads for each task, and calls them without any delays. The yielding in the connector implementation will allow the scheduler to process other threads while the current one is busy. With the blocking socket types, the connectors read with a 0 timeout to ensure to never block, and return an idle-signal, and the io-loop should take care of "sleeping" enough to prevent a busy-loop.

Note that the change is NOT complete yet.

Tieske commented 2 years ago

I also added logging (using LuaLogging), not required for library use, but is required when running the tests on this branch.

(actually had to write a new LuaLogging appender to enable it for the MQTT nginx connector)

xHasKx commented 2 years ago

Wow, that's huge work, thanks for your efforts! And it looks like there's huge review work ahead :)

Tieske commented 2 years ago

yes, a bit more than anticipated πŸ˜„

anyway, I don't expect it to be fully functional yet. Just starting to implement some devices, that will probably be the proof-of-the-pudding. (it would also be a final step in testing the Copas beta, so that can be released as well)

Tieske commented 2 years ago

πŸŽ‰ all green now πŸ˜„

Tieske commented 2 years ago

@xHasKx some review comments would be nice if you can spare some time.

I need to review the nginx examples, as they probably suffer from the same problems as the copas one (and then there's a host of nginx specifics to watch for)

When that is done, docs need checking and updating.

Tieske commented 2 years ago

Sync loop can be used in the simplest mqtt case - connect, subscribe, publish command, receive answer, disconnect. This zero-depenndence scenario have to be preserved.

The differences between the ioloop and sync loop examples are negligible. And since ioloop is available, the zero-dependency scenario is still working. Right?

fperrad commented 1 year ago

@Tieske take a look at https://fperrad.frama.io/lua-mqtt/examples/, socket & event loop features stay outside the protocol.

Tieske commented 1 year ago

@fperrad nice, that might a good option. Despite the improvements made I'm still seeing race conditions where different threads access the socket simultaneously with this implementation. And it's just hard to keep that code loosly-coupled, and functional.

Update: the issues were related to bugs in Copas (scheduler) and since fixing them this branch has been running without issues.