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

MQTT V5 support #334

Merged
merged 35 commits into from Sep 24, 2019
Merged

MQTT V5 support #334

merged 35 commits into from Sep 24, 2019

Conversation

@icraggs
Copy link
Collaborator

@icraggs icraggs commented Sep 26, 2018

These are my initial changes for MQTT V5 support. They allow V5 packets to be sent and received, as shown in the test_mqttv5.py. I'm submitting the changes at this stage for any comments or suggestions before I continue.

Remaining work:

  • V5 functionality: AUTH packets (probably for later), handling incoming DISCONNECT packet, session expiry (clean start vs clean session), server receive maximum (flow control)
  • doc
  • tests
  • examples
Ian Craggs
@icraggs icraggs added the enhancement label Sep 26, 2018
@icraggs icraggs requested a review from PierreF Sep 26, 2018
@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Sep 26, 2018

Oh, yes. It only works in Python 3 at the moment, I just found out. Looking into it.

@AtosNicoS

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS commented on src/paho/mqtt/client.py in d2d62a1 Sep 27, 2018

Just for maintainablity:
Wouldnt it be better to define a small/simple (possibly inline) function that is ether called with a string or the list gets iterated and then the function is called with eath list entry string?

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Sep 27, 2018

I made minimal changes to the underlying code - I that's a question for Pierre.

@AtosNicoS

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS commented on src/paho/mqtt/client.py in d2d62a1 Sep 27, 2018

The previous version of the function was way simpler and the command in the return statement different. I do not know any details about how it work. But did you made sure this code still works with mqtt v3? I am sure it is, just questioning it ;-)

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Sep 27, 2018

MQTT Version 3 of disconnect has no reason code or properties, basically no parameters, that's why it's simple. It should work with version 3 - I ran the tests I could find, but the CI tests should tell.

@AtosNicoS

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS commented on src/paho/mqtt/client.py in d2d62a1 Sep 27, 2018

This :2 Addition is new, also for mqtt3. Is it still correct?

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Sep 27, 2018

Yes!

@AtosNicoS

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS commented on src/paho/mqtt/packettypes.py in d2d62a1 Sep 27, 2018

I already asked about enums here. I was not sure if your old comment intendet to change this code to enums or to keep it as it is. No matter what, this is just a reminder.

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Sep 27, 2018

Enums are a fairly recent addition, apparently. Depends what version(s) of Python we want to support.

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS replied Sep 27, 2018

Enums were added in Python 3.4
Debian Jessie (current is stretch) uses Python 3.4.x and Ubuntu 16.04 (current is 18.04) uses Python 3.5.x.
Python 3.3 is end of life: https://www.python.org/dev/peps/pep-0398/#x-end-of-life
So I do not see any problem is using enums.

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Sep 27, 2018

Are they in Python 2.7?

@AtosNicoS

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS commented on src/paho/mqtt/properties.py in d2d62a1 Sep 27, 2018

As said before, I'd rename the whole class rather than making a comment.

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Sep 27, 2018

I've made that change in my latest update - thought I'd put it into this commit, apparently not.

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Apr 9, 2019

Fixed in latest commit.

@AtosNicoS

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS commented on src/test_mqttv5.py in d2d62a1 Sep 27, 2018

What is the reason for specifying the disconnect type twice or even once? A disconnect call should know it is a disconnect. I might missunderstand, but I dont understand the difference between the reasoncodes and the properties.

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Sep 27, 2018

Reasoncodes

http://docs.oasis-open.org/mqtt/mqtt/v5.0/cs02/mqtt-v5.0-cs02.html#_Toc514345303

and properties:

http://docs.oasis-open.org/mqtt/mqtt/v5.0/cs02/mqtt-v5.0-cs02.html#_Toc514345299

are part of the MQTT 5.0 specification. Different subsets of reason codes and properties are applicable to certain packet types. Specifying the packet type on the constructors allows validation.

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS replied Sep 27, 2018

I think i got a better understanding now. The aName is possibly also a candicate for enums.

@AtosNicoS

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS commented on src/test_mqttv5.py in d2d62a1 Sep 27, 2018

Same question here: Why is specifying the unsubscribe type here required. I assume that this is an internal requirement, but I'd rather design the api to pass a dictionary to the function and the API builts the property object and appends the passed in user properties. From a user perspective this sounds easier for me.

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS replied Sep 27, 2018

Or if it is really required an alternative would be to create a wrapper class named UnsubscribeProperties, which might be clearer. You must know best.

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Sep 27, 2018

Knowing the packet type for the properties object allows the properties to be validated. Only a subset of properties are allowed for each packet type.

@AtosNicoS

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS commented on src/test_mqttv5.py in d2d62a1 Sep 27, 2018

What does the number 2 stand for?

This comment has been minimized.

Copy link
Owner

@icraggs icraggs replied Sep 27, 2018

It's in the MQTT standard for retainHandling. We could have constants/enums for the values.

http://docs.oasis-open.org/mqtt/mqtt/v5.0/cs02/mqtt-v5.0-cs02.html#_Toc514345441

0 = Send retained messages at the time of the subscribe
1 = Send retained messages at subscribe only if the subscription does not currently exist
2 = Do not send retained messages at the time of the subscribe

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS replied Sep 27, 2018

Those 3 number should be put into an enum.

@AtosNicoS

This comment has been minimized.

Copy link

@AtosNicoS AtosNicoS commented on src/test_mqttv5.py in d2d62a1 Sep 27, 2018

A loop_stop() might be missing here.

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Oct 1, 2018

OK, I've made this work in Python2.7 as well as 3. The tests are failing because they need an MQTT v5 broker to work against - this can be achieved by running the Paho test broker as does the C client. Is this a good option, or would you prefer the new tests to be run in the same way as existing?

Copy link
Contributor

@PierreF PierreF left a comment

New files you created use 2 spaces for indentation. Could you use 4 spaces instead ?

I hadn't yet time to look on the MQTT v5 protocol itself (i.e. I hadn't run anything with a v5 broker). I will try to made some test in the following week.

Thanks for this contribution.

src/paho/mqtt/client.py Outdated Show resolved Hide resolved
src/paho/mqtt/client.py Show resolved Hide resolved
src/paho/mqtt/client.py Outdated Show resolved Hide resolved
src/paho/mqtt/client.py Show resolved Hide resolved
src/paho/mqtt/properties.py Outdated Show resolved Hide resolved
src/paho/mqtt/properties.py Outdated Show resolved Hide resolved
src/paho/mqtt/properties.py Outdated Show resolved Hide resolved
@PierreF
Copy link
Contributor

@PierreF PierreF commented Oct 7, 2018

About the test, I think it would be better to use same kind of tests as already present.
There is already two kind of test: "test" folder which use old tests and "tests" folder which use new testing infrastructure (py.test) but we hadn't moved all tests yet.

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Oct 8, 2018

About the test, I think it would be better to use same kind of tests as already present.
There is already two kind of test: "test" folder which use old tests and "tests" folder which use new testing infrastructure (py.test) but we hadn't moved all tests yet.

I didn't expect that test to run - that was purely for checking that this code actually works before adding some proper tests!

Up to now on the other clients I've always used the Python test broker to test against, but I didn't know whether you would be happy with that approach. See https://github.com/eclipse/paho.mqtt.c/blob/master/travis-install.sh for an example. I think it provides some more flexibility and complicated tests if they are run against a 'real' broker such as this. What do you think?

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Oct 8, 2018

Yes, I'll change to 4 space indentation. I have always used 2 spaces myself to maximise space on the right hand side :-)

@AtosNicoS
Copy link

@AtosNicoS AtosNicoS commented Nov 9, 2018

Is there any update on this? :)

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Jan 9, 2019

Sorry, I'm working on the Java client release (https://github.com/eclipse/paho.mqtt.java/milestone/2) as my top priority. I'll get back to this as soon as I can. I'm open to any offers of help.

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Apr 9, 2019

I've fixed the indentation, added constant definitions for the retain handling options, and removed some debug statements. The simple validation test works for me.

I'd like to know what you would like to see from me for this PR to be accepted. Thanks!

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Apr 10, 2019

Oh - obviously I should make the tests work! And there are some other comments I haven't yet addressed.

Ian Craggs added 4 commits May 2, 2019
Ian Craggs
Ian Craggs
@icraggs icraggs requested a review from PierreF May 28, 2019
@PierreF
Copy link
Contributor

@PierreF PierreF commented May 29, 2019

Apart from running test that may need some improvement (if I run tox on my system, it will fail because the testing/interoperability broker is not running), I don't have more changes to do.

Are you going to make documentation change in this PR ?

About running the test, I'm not sure what is the best solution... I think to some kind of conditional test, like "tox --enable-interoperability-tests" which would be used by travis or someone which run the interop broker. But I don't known what the proper way to do this.
If you had time to look at this, it would be great. If not, I'll fix it later (I think using an environment variable and pytest skipif may do the job).

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented May 29, 2019

@PierreF Thanks. Yes, it is my intention to add the docs in this PR, I just wanted to get your feedback before doing that. Did you get chance to try these changes out with Mosquitto?

I'll look into getting the tox tests running without having to run the interop broker separately - it hadn't occurred to me.

@PierreF
Copy link
Contributor

@PierreF PierreF commented May 29, 2019

No, I didn't tested them with Mosquitto. But the test_mqtt5 with interop broker seems to have good coverage of MQTT 5. That still on my todo, but I don't think it's mandatory before merging this PR.

@ralight
Copy link
Contributor

@ralight ralight commented May 29, 2019

I've run test_mqtt5 against Mosquitto and it all seems fine, apart from a few issues with Mosquitto which have been highlighted and mostly fixed.

Having said that, I'm seeing some failures in test_client_topic_alias and test_session_expiry when I run the whole suite but that run fine on their own.

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented May 29, 2019

@ralight there are likely still some timing foibles in the tests, which I think are down to the way the tests are written rather than the client or server behaviour errors. I found stopping and restarting the background thread loop often quite unwieldy for the tests I wanted to write. Maybe there's a better way.

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented May 29, 2019

And @ralight, while you're here :-) I wonder if you have a view on this:

In MQTT 5.0, the session cleanup is governed by the session expiry property, which defaults to 0. This means that if you want to clean up at the start of the session, but be able to reconnect and continue if there are communication breaks until your work is done, you can do it in one go:

connect clean_start=true, session_expiry=99999 (value in seconds)

disconnect also has a session expiry property defaulting to 0, so when you issue a normal disconnect, then the session is cleaned up immediately.

So I think we could do with some setting which means "use clean start on the first connect, but not on subsequent re-connects". This could be implied from the session expiry value and session present connack flag, although that might be trying to be too clever?

@ralight
Copy link
Contributor

@ralight ralight commented May 29, 2019

@icraggs I agree, the current reconnection process of using the same flags as set at the initial connection is limiting. I also agree that implying it from the session expiry value would be being too clever. I don't know what the best way of presenting it though, perhaps a further argument to the connect() call?

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented May 29, 2019

It might be that the natural default should be that clean_start=true only applies to the first connect and not to subsequent reconnects, as that is what I think most people would want most of the time. Then the other option could be clean_start_always or similar, defaulting to false, and not allowed together with clean_start. Or clean_start could take the possible values [True, False, FIRST_ONLY] in some way, defaulting to FIRST_ONLY.

I think that these should be on the connect call rather than the constructor, as the connect properties are on the connect also (as I have implemented it). The only downside as far as I see it is that this could be a bit confusing for people migrating from V3.

@ralight
Copy link
Contributor

@ralight ralight commented May 29, 2019

I'd support a three value option, and think it would be better in the connect() call. clean_session was in the constructor because it applied to the whole session, that's not necessarily true any more.

Ian Craggs added 2 commits Jun 10, 2019
@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Jun 11, 2019

I've added the three value clean_start option, and put it on connect rather than the constructor. I've also added documentation for the MQTT v5.0 functionality. So I think that's all done, apart from the question of making the V5 tests run with tox only.

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Jun 11, 2019

@PierreF I've moved the test broker startup to tox.ini, so the tests work when you use tox outside of Travis too. The only situation that won't work is if one of the ports the test broker (like 1883) is already in use, probably by another MQTT broker like Mosquitto.

So that's all done from my point of view.

@fpagliughi
Copy link
Contributor

@fpagliughi fpagliughi commented Jul 11, 2019

Would it be possible to maybe accept this PR into a new 'mqttv5' branch (or something like that)?

Doing so might make it easier for other people to find, test, comment upon, or possibly fix and enhance.

PierreF added 3 commits Jul 14, 2019
This allow code to still perform "reasonCode == 0" as is was when "rc"
was used.
And suggest to add properties=None for MQTTv3. This makes easier to use
the same callback for MQTTv3 & MQTTv5.
This match granted_qos behavior
@PierreF
Copy link
Contributor

@PierreF PierreF commented Jul 17, 2019

@icraggs as discussed by email, the last point I wanted to improve was to make easier to have callbacks that works for both MQTTv3 / MQTTv5.

The changes were rather minor, so I pushed commit on your branch. I hope you don't mind.

The changes are indented to make on_connect/on_subscribe easier to works with both MQTTv3 and MQTTv5:

  • rc (and interger) / reasonCode (a object) could be compared to interger. So rc == 0 and reasonCode == 0 is valid
  • Suggested to add properties=None as last parameter. So the callback could be called without properties (MQTTv3) or with properties (MQTTv5)
  • Change to always provide a list of reasonCodes in on_subscribe. Since reasonCode are comparable to integer, it will behave like granted_qos (reasonCodes == [0] will works)

If my changes looks good to you, I'm going to merge it into master.

@PierreF
Copy link
Contributor

@PierreF PierreF commented Jul 18, 2019

Added "six" in test dependency... we use it since addition of pytest IIRC. I think some 3th party dependency (I think to pylama) required six but now only require it with Python below 3.5.

@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Jul 18, 2019

@PierreF the changes look good to me. Thanks!

@PierreF PierreF merged commit 91e541f into eclipse:master Sep 24, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
@icraggs
Copy link
Collaborator Author

@icraggs icraggs commented Sep 24, 2019

Thanks @PierreF !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.