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

mosquitto_pub message-expiry-interval usage #1546

Closed
kiranpradeep opened this issue Dec 31, 2019 · 4 comments
Closed

mosquitto_pub message-expiry-interval usage #1546

kiranpradeep opened this issue Dec 31, 2019 · 4 comments

Comments

@kiranpradeep
Copy link

@kiranpradeep kiranpradeep commented Dec 31, 2019

Description

I am trying to use mosquitto_pub/mosquitto_sub to test message-expiry-interval property. It appears there is bug in implementation of mosquitto_pub or _sub. I tried using other language clients with mosquitto and message expiry worked correctly. But some thing appears wrong with mosquitto_pub.

Version

Mosquitto: 1.6.8
Mosquitto Pub: mosquitto_pub version 1.6.3 running on libmosquitto 1.6.3.
OS: macOS 10.13

Steps.

  1. mosquitto_sub -t T -c -i X -q 2 -V mqttv5 //create persistent session for client X
  2. mosquitto_pub -D publish message-expiry-interval 100 -t T -m hello -q 2 -V mqttv5 //publish message A with expiry of 100 seconds
  3. mosquitto_sub -t T -c -i X -q 2 -V mqttv5 //expect reception of message hello

Here we receive no message in step 3.

@ralight

This comment has been minimized.

Copy link
Contributor

@ralight ralight commented Jan 29, 2020

This is an interesting case. The problem comes about because in MQTT v5, the meaning of the clean session flag changed. In MQTT v3.1 and v3.1.1 when the flag is set to 0 it means both "do not clean an existing session when I log in", and "when I disconnect, do not clean my session". In v5 when it is set to 0, it only means "do not clean an existing session when I log in". The removal of the session when the client disconnects is handled by the "session expiry interval" property, which can be set on the connect or disconnect commands.

The mosquitto_pub and _sub tools follow this behaviour, so your first mosquitto_sub command will expire immediately after you quit. If you use the command below then it will work as you expect.

mosquitto_sub -t T -c -i X -q 2 -V mqttv5 -D connect session-expiry-interval 100

Now to my mind this is a bug because we've changed the expected behaviour of the -c argument depending on whether it is a v3 or v5 client. I think it should be changed to match v3, and another easier way of setting the session expiry added. I've summarised the different combinations of clean start and session expiry in the table below, along with how you would achieve that effect now, plus a proposed new command line argument for session expiry. What do you think?

clean start expiry interval current args proposed args
1 0 None None / -x 0
1 N -D connect session-expiry-interval N -x N
1 Infinite -D connect session-expiry-interval 4294967296 -x -1
0 0 -c -c -e 0
0 N -c -D connect session-expiry-interval N -c -x N
0 infinite -c -D connect session-expiry-interval 4294967296 -c / -c -x -1
@kiranpradeep

This comment has been minimized.

Copy link
Author

@kiranpradeep kiranpradeep commented Jan 29, 2020

Thanks for the detailed explanation which helped in correct understanding of clean session flag. I could be easily wrong on any of below, please feel free to do it the way you feel best without further explanations. I did little reading on clean start/session. This article [1] gave an impression like: it is not that meaning of clean session has changed. I felt more like clean session is no more part of v5. What is part of v5, is clean start [2]. Based on that view, what I prefer will be - to be more explicit

  1. The flag -c retains its existing meaning for v3. But, usage of flag -c with v5 will throw friendly error message
  2. New flag introduced for mqttv5: --cleanstart which corresponds of clean start. Usage of --cleanstart with v3 will throw friendly error message.
  3. Usage of flag -x as defined for v5.

[1] https://medium.com/@emqtt/mqtt-5-0-new-feature-clean-start-and-session-expiry-interval-748d4b1a2a95
[2] https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901039

@ralight

This comment has been minimized.

Copy link
Contributor

@ralight ralight commented Jan 30, 2020

Interestingly -c meant "disable clean start" rather than "disable clean session" originally for MQTT v3, before it changed to clean session in MQTT v3.1.

I disagree with your suggestion for -c and --cleanstart, I think it is confusing to have different behaviour for the two different protocol versions. With my suggestion, -c does the same thing for both protocol versions - disable cleaning of sessions on connect, and setting the session to never expire. The fact that the mechanism that it achieves this is different in the two protocol versions is not important. Likewise not providing -c behaves the same in both cases, with sessions being cleaned on connect, and sessions expiring immediately on disconnect. Consistency of behaviour is really really important, you'll have to have a very compelling argument to persuade me otherwise.

ralight added a commit that referenced this issue Feb 6, 2020
Default behaviour for v5 clients using `-c` is now to use infinite length
sessions, as with v3 clients.

Closes #1546. Thanks to Kiran Pradeep.
ralight added a commit that referenced this issue Feb 6, 2020
Default behaviour for v5 clients using `-c` is now to use infinite length
sessions, as with v3 clients.

Closes #1546. Thanks to Kiran Pradeep.
ralight added a commit that referenced this issue Feb 6, 2020
Default behaviour for v5 clients using `-c` is now to use infinite length
sessions, as with v3 clients.

Closes #1546. Thanks to Kiran Pradeep.
@ralight

This comment has been minimized.

Copy link
Contributor

@ralight ralight commented Feb 6, 2020

I think that is all fixed now, so I'm going to close this.

@ralight ralight closed this Feb 6, 2020
michaeliu added a commit to michaeliu/mosquitto that referenced this issue Feb 7, 2020
commit f16d9e2
Author: Roger A. Light <roger@atchoo.org>
Date:   Thu Feb 6 21:05:52 2020 +0000

    Add file missing from earlier commit.

commit c4e41f3
Author: Roger A. Light <roger@atchoo.org>
Date:   Thu Feb 6 16:43:29 2020 +0000

    Back port db_dump from develop.

    Closes eclipse#1519. Thanks to Christoph Krey.

commit 2a8c1d0
Merge: 17e20de 4408339
Author: Roger A. Light <roger@atchoo.org>
Date:   Thu Feb 6 16:20:52 2020 +0000

    Merge branch 'coverity-fixes' into fixes

commit 17e20de
Author: Roger A. Light <roger@atchoo.org>
Date:   Thu Feb 6 16:12:29 2020 +0000

    Fix session-expiry-interval for v5 clients using -c.

    Default behaviour for v5 clients using `-c` is now to use infinite length
    sessions, as with v3 clients.

    Closes eclipse#1546. Thanks to Kiran Pradeep.

commit 078ad75
Author: Gianfranco Costamagna <costamagnagianfranco@yahoo.it>
Date:   Wed Jan 22 12:29:41 2020 +0100

    cmake: add ADNS enable/disable dynamic support

    Signed-off-by: Gianfranco Costamagna <costamagnagianfranco@yahoo.it>

commit e9a7150
Author: Gianfranco Costamagna <costamagnagianfranco@yahoo.it>
Date:   Wed Jan 22 12:31:01 2020 +0100

    Bugfix: enabling DLT was overriding everything else on linker flags because of error in cmake set keyword

    Signed-off-by: Gianfranco Costamagna <costamagnagianfranco@yahoo.it>

commit 7a5c2d4
Author: Gianfranco Costamagna <costamagnagianfranco@yahoo.it>
Date:   Wed Jan 22 12:30:25 2020 +0100

    Bugfix: include "deps" directory only if BUNDLED_DEPS has been provided and set to true

    Signed-off-by: Gianfranco Costamagna <costamagnagianfranco@yahoo.it>

commit 56d0b95
Author: Roger A. Light <roger@atchoo.org>
Date:   Wed Feb 5 15:19:55 2020 +0000

    Fix `--remove-retained` not obeying the `-T` option.

    This means `--remove-retained -t bbc/# -T bbc/one/#` would remove all
    retained messages in `bbc/#`, instead of leaving all of the topics in
    `bbc/one/#`.

    Closes eclipse#1585. Thanks to Simon Moser.

commit 3a89059
Author: Roger A. Light <roger@atchoo.org>
Date:   Tue Feb 4 17:11:11 2020 +0000

    Don't call SSL_shutdown() if SSL init hasn't completed.

commit 07c5462
Author: Roger A. Light <roger@atchoo.org>
Date:   Tue Feb 4 16:59:29 2020 +0000

    Print OpenSSL errors in more situations

    Covers when loading certificates fails, or there are ENGINE problems.

    Closes eclipse#1552. Thanks to Michael Richardson.

commit 27b4518
Author: Roger A. Light <roger@atchoo.org>
Date:   Tue Feb 4 16:05:58 2020 +0000

    Improve password file parsing in the broker and mosqitto_passwd.

    Closes eclipse#1584. Thanks to panava.

commit 4408339
Author: Roger A. Light <roger@atchoo.org>
Date:   Thu Jan 23 12:51:47 2020 +0000

    Make consts unsigned where they are compared against unsigned.

commit 5528dde
Author: Roger A. Light <roger@atchoo.org>
Date:   Thu Jan 23 12:51:12 2020 +0000

    Fix possible null dereferences.

commit 05ec02b
Author: Roger A. Light <roger@atchoo.org>
Date:   Thu Jan 23 10:55:49 2020 +0000

    Remove dead values.

commit 18f0508
Author: Roger A. Light <roger@atchoo.org>
Date:   Thu Jan 23 10:07:56 2020 +0000

    Fix dereference before null check.

    Coverity Scan 1405815.

commit db62f98
Author: Roger A. Light <roger@atchoo.org>
Date:   Thu Jan 23 09:35:28 2020 +0000

    Fix unused value being overwritten.

    Coverity Scan 1400727.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.