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

[Bug]: Outgoing QoS upgrades not affecting storage of offline messages #2220

Closed
1 task done
micw opened this issue Nov 23, 2023 · 5 comments
Closed
1 task done

[Bug]: Outgoing QoS upgrades not affecting storage of offline messages #2220

micw opened this issue Nov 23, 2023 · 5 comments
Labels

Comments

@micw
Copy link

micw commented Nov 23, 2023

Environment

  • VerneMQ Version: 1.13.0
  • OS: Linux/Docker
  • Cluster size/standalone: Standalone

Current Behavior

I run VerneMQ with the following command:

docker run -it --rm \
 -e DOCKER_VERNEMQ_ACCEPT_EULA=yes \
 -e DOCKER_VERNEMQ_ALLOW_ANONYMOUS=on \
 -e DOCKER_VERNEMQ_UPGRADE_OUTGOING_QOS=on \
 -p 1883:1883 -p 8888:8888 vernemq/vernemq:1.13.0

I create a persistent offline session with

mosquitto_sub -t 'test1' -q 1 -i c1 -c -d

Then I terminate the client and send two messages:

mosquitto_pub -t "test1" -m "test.q0" -q 0
mosquitto_pub -t "test1" -m "test.q1" -q 1

Then I start the client again:

mosquitto_sub -t 'test1' -q 1 -i c1 -c -d
Client c1 sending CONNECT
Client c1 received CONNACK (0)
Client c1 sending SUBSCRIBE (Mid: 1, Topic: test1, QoS: 1, Options: 0x00)
Client c1 received PUBLISH (d0, q1, r0, m1, 'test1', ... (7 bytes))
Client c1 sending PUBACK (m1, rc0)
test.q1
Client c1 received SUBACK
Subscribed (mid: 1): 1

Expected behaviour

Since QoS is upgraded from 0 to 1, I expect both messages to be handled as offline messages and sent to the client after it re-connects.

Configuration, logs, error output, etc.

Nothing of relevance

Code of Conduct

  • I agree to follow the VerneMQ's Code of Conduct
@micw micw added the bug label Nov 23, 2023
@ioolkos
Copy link
Contributor

ioolkos commented Nov 23, 2023

@micw thanks, something smells wrong here.
I think we fail to add a message id and do not add the "QoS-0-message-which-is-now-a-Qos>0-message" to the waiting acks and replay queue in the FSM statemachines.
I agree the behaviour you expect is what the option promises. So, should be somehow fixed and aligned, even if it's a non-spec option. ("We have left MQTT jurisdiction").

This is somewhat low-priority for me but feel free to propose a fix, if it's urgent for your use case.


👉 Thank you for supporting VerneMQ: https://github.com/sponsors/vernemq
👉 Using the binary VerneMQ packages commercially (.deb/.rpm/Docker) requires a paid subscription.

@mths1
Copy link
Contributor

mths1 commented Nov 24, 2023

The upgrade is done too late (the decision to store on disk or not has already been made). The upgrade should be moved to the earliest possible moment (maybe in vmq_queue, and not in mqtt_fsm)

@ioolkos
Copy link
Contributor

ioolkos commented Nov 26, 2023

@mths1 Thanks for the PR! @micw can you give it a try? #2221


👉 Thank you for supporting VerneMQ: https://github.com/sponsors/vernemq
👉 Using the binary VerneMQ packages commercially (.deb/.rpm/Docker) requires a paid subscription.

@micw
Copy link
Author

micw commented Nov 27, 2023

Test plan:

  1. publish without client connected and without any sessions
mosquitto_pub -t "test1" -m "test.q0" -q 0
mosquitto_pub -t "test1" -m "test.q1" -q 1
  1. subscribe with a client, qos=1, clear-session=false
mosquitto_sub -t 'test1' -q 1 -i c1 -c -d

Expected result: no messages are received - PASS

  1. publish with client connected
mosquitto_pub -t "test1" -m "test.q0" -q 0
mosquitto_pub -t "test1" -m "test.q1" -q 1

Expected result: both messages are received by the connected client. Both have qos 1 - PASS

  1. disconnect the client, publish with client disconnected and existing offline session
mosquitto_pub -t "test1" -m "test.q0" -q 0
mosquitto_pub -t "test1" -m "test.q1" -q 1
  1. subscribe with the same client id again, qos=1, clear-session=false
mosquitto_sub -t 'test1' -q 1 -i c1 -c -d

Expected result: both offline messages are received by the client. Both have qos 1 - PASS

Works for me.

@ioolkos
Copy link
Contributor

ioolkos commented Nov 27, 2023

@micw thanks for verifying! cc @mths1


👉 Thank you for supporting VerneMQ: https://github.com/sponsors/vernemq
👉 Using the binary VerneMQ packages commercially (.deb/.rpm/Docker) requires a paid subscription.

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

No branches or pull requests

3 participants