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

Initial commit for MQTT v3.1.1 support #104

Closed
wants to merge 1 commit into from

Conversation

viscropst
Copy link

@viscropst viscropst commented Nov 10, 2022

A solution for issue #89 ,#6. aid to using paho.golang for MQTT V3.1.1

Signed-off-by: Yu Zhu <bcct2010@live.cn>
@ewirch
Copy link
Contributor

ewirch commented Mar 12, 2023

@viscropst, why don't you use github.com/eclipse/paho.mqtt.golang for v3 MQTT?

@mvrhov
Copy link

mvrhov commented Mar 12, 2023

Because the interface of that library is horrible and it looks like it was done by someone who has never seen go before.

@MattBrittan
Copy link
Contributor

MattBrittan commented Mar 12, 2023

I agree that the interface of the V3.1.1 library is not as good (this library was started afresh partly because of the issues with the V3 client API). However the V3 client is a more complete implementation of the protocol (V5 client does not yet support storing session state; so publishing at QOS1+ has no benefit) so I'd still recommend it for most users (if you want to use the V5 client then it's important to understand its limitations). The V3 client is now stable (I've had instances running constantly for over a year on unstable links) and the reality is that most programs will probably only reference the library on 5-10 lines so, the sub-optimal API is probably not a massive issue.

Ref this specific issue; I had been waiting for @alsm to comment here, but as it's been a few months (and there has been some reaction) I'll add my thoughts. As you noted a request for V3 functionality was made some time ago (#6 - V3 client in the same style) and this was closed by @alsm with:

What I'm saying is that superficially it might seem like only a few changes to make this client support 3.1.1 but it will actually be a lot more work to make a client that is fully conformant to both specs. I don't think it can be reasonably done in the same codebase. If @viscropst wants to try I'm more than happy to be proven wrong 😁 and if it works will happily accept it, but what I don't want is to make the current v5 client harder or more awkward to use.

My biggest concerns are the complexity of use and the complexity of further development (the lesson learnt from the V3 client is that seemingly simple options can be a lot of work to maintain and make future enhancements much more difficult). As such I'm keen for the V5 client to remain as simple as possible (whilst fully supporting MQTT V5).

I personally feel that implementing session state (persistence) should be the highest priority for this project (because currently the client does not fully implement the MQTT spec as it stands) and am concerned that implementing V3 will make this more difficult (there are some subtle differences in the specs). This will be a fairly big change and I have put some time looking into approaches (but unfortunately something else always came up before I was able to finish an implementation).

If a V3 client with this API is desired then I'd prefer it to be implemented as a sub project that is implemented in the same style as the V5 client (so you can just swap the import path to get the V3 client) - perhaps with a package providing a connect function that supports both. This would lead to more duplication of code but would remove the need for all of the if !isVer4() and avoid limiting the V5 client. Should both clients be implemented together (as per this PR) then I think we would need to accept that this is a V5 first client (and V3 is a bit of a second class citizen because guidance from the V5 spec will be followed by default).

I have not looked at the implementation in detail (sorry) but did note that it appears to be using a global variable (packets/_protocolVersion) which I'm not keen on (will work until someone tries to bridge MQTTV3 and V5 brokers with the client and looks like it's technically a race condition when there are two broker connections). I'll see what further feedback this gets (and whether @alsm responds) before considering looking further (Al created this client and I just added the AutoPaho part so had only been looking at PR's in that area).

@viscropst - thanks very much for your work on this and I do hope that we can use it; my caution is mainly due to the many hours I've spent trying to untangle the V3.1.1 client (and once you add a feature it's very hard to remove it!).

@mvrhov
Copy link

mvrhov commented Mar 12, 2023

I haven't looked deeply into the library. I've just tested both libs v3 and v5 (Because the MQTT project was put on hold). But IMHO you should provide only interface and if one really needs persistence, than they should implement it themselves.
There is nothing that prevent this library then to provide a persistence as a different package.

@MattBrittan
Copy link
Contributor

if one really needs persistence, than they should implement it themselves

Sorry - the terminology I used may have been confusing (I have edited my response to clarify). By persistence I mean storing the session state (see 4.1.1 in the spec). MQTT supports various QOS levels enabling you to guarantee delivery/receipt; this client does not currently support this feature (it will allow you to specify QOS1/2 but will never resend a message and will just respond yes when asked for confirmation if it has received a message). See issue #25 for more info.

For some use-cases (i.e. when the latest value is all you are interested in) this is fine, but if missing messages is a problem, then you cannot currently rely on this client (it's why I have not moved to it yet).

if one really needs persistence, than they should implement it themselves. There is nothing that prevent this library then to provide a persistence as a different package.

Unfortunately it's fairly complex to implement and tied deeply into the protocol. The library would need major modifications to support a third party persistence package and such code would need to be pretty tightly coupled (i.e. when receiving an acknowledgment the library needs to know about the message the acknowledgment refers to so it can respond correctly). This is not something that a normal user could, or should be expected to, implement (I have had a few goes; the challenge is implementing it in a way that does not add too much complexity).

@alsm
Copy link
Contributor

alsm commented May 24, 2023

I appreciate the effort you've put in to doing this but while this offer superficial MQTT 3.1.1 support there are differences in behaviour that will not be reflected and make it not a replacement for other 3.1.1 clients. As such I think 3.1.1 support is specifically not going to be a feature of this library.

@alsm
Copy link
Contributor

alsm commented Jun 13, 2023

Closing as 3.1.1 support is no longer something I plan to implement

@alsm alsm closed this Jun 13, 2023
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

Successfully merging this pull request may close these issues.

5 participants