Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding a streaming interface #32

Closed
Tieske opened this issue Oct 26, 2021 · 4 comments
Closed

adding a streaming interface #32

Tieske opened this issue Oct 26, 2021 · 4 comments

Comments

@Tieske
Copy link
Contributor

Tieske commented Oct 26, 2021

Hi @xHasKx

Thx for this nice library, I've been eyeballing MQTT for a while now, and thought to give it a go actually.

The code looks nice and clean, but what I'm missing is a streaming interface. Looking at the code, currently client.lua mixers client functionality with io-loop functionality. Which isn't the right separation of concerns imho.

So would you be interested in a streaming interface? I've seen another issue about blocking IO (#23 ). So what I would propose to separate the client from the runloop is along the following lines;

  • new method client:consume_bytes(byte_string). This means the client is no longer responsible for reading data, only for processing it. Any runloop can read data in whatever way it sees fit, and then feed that into this method.
  • new callback provided to the client; send_bytes(byte_string). Whenever the client needs to send data, it calls this callback with the data to be transmitted.

This way the client and the runloop are cleanly decoupled.

wdyt?

as a start I sent a PR (#31 ) to expose the keepalive functionality to external runloops.

@xHasKx
Copy link
Owner

xHasKx commented Oct 29, 2021

@Tieske, please take a look at the Connectors topic - https://github.com/xHasKx/luamqtt#connectors
They're providing .send() and .receive() methods used by a MQTT protocol parsing code run in the coroutine like this: https://github.com/xHasKx/luamqtt/blob/master/mqtt/protocol5.lua#L811
That approach allows me to write the protocol parsing code much cleaner than with any callbacks.

read_func (recv_func) is created inside MQTT client code here:
https://github.com/xHasKx/luamqtt/blob/master/mqtt/client.lua#L751
or here:
https://github.com/xHasKx/luamqtt/blob/master/mqtt/client.lua#L1087

For now, that approach allows me to integrate several connectors from different runtime environments (luasocket, copas, nginx).
So, the level of client and run-loop coupling is quite low IMO.

But of course you can suggest a better way to decouple network streams reading logic, if it leave the MQTT protocol parsing code in the same form.

@Tieske
Copy link
Contributor Author

Tieske commented Nov 1, 2021

That approach allows me to write the protocol parsing code much cleaner than with any callbacks.

Yes, after spending quite some time reading the code, I noticed that.

But I also ran into some other issues, there are hardcoded error messages in the client.lua code, that are connector specific. That is not safe. For example here. Sometimes a "timeout" is an actual error, and sometimes, it is a notification that reading on the socket should be retried later. (not to mention that "wantwrite" is missing there).

So, the level of client and run-loop coupling is quite low IMO.

The code in client.lua has 2 implementations, a sync one and the ioloop one, that code belong in the repspective loops imo, not the client.

@Tieske
Copy link
Contributor Author

Tieske commented Nov 1, 2021

So I've been updating code, but unfortunately the changes were bigger than I anticipated (sorry about that 😞 ).

I'll push the code to #31 shortly.

@Tieske
Copy link
Contributor Author

Tieske commented Nov 1, 2021

closing this in favor of #31

@Tieske Tieske closed this as completed Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants