publish / subscribe / unsubscribe methods return a notice future that waits publish (QoS0), PubAck (QoS1) and PubRec (QoS2) #851

xiaocq2001 · 2024-04-26T06:42:45Z

Type of change

Added reason code for PubAck/Rec/Comp.
Fix notification flow for NoticeFuture and Eventloop.
See discuss #805

Checklist:

Formatted with cargo fmt
Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

* fix: v5 doesn't write outgoing packets onto network bytebeamio#825 (comment) * same for pingreq

…d_test

FSMaxB

Just a question

FSMaxB · 2024-06-19T13:07:39Z

rumqttc/src/state.rs

+        // Notify user about the publish, pubrel and pubcomp will be handled in background
+        if let Some(tx) = tx  {
+            tx.success();
+        }


What is "handled in the background" supposed to mean in this context.

From what I understand, this makes the NoticeFuture complete on PubRec instead of PubComp for ExactlyOnce QoS, which is the wrong behavior from what I understand.

IMO the notice is to confirm that the message actually received by broker.
So in QoS0 it's noticed on message sent (since there is no ACK), in QoS1 it's noticed on ACK reception, in QoS2 it's noticed on REC which indicates broker accept the message. The PubRel and PubComp do not affect the broker reception of message, but some way to let broker release QoS2 message.

QoS2 it's noticed on REC which indicates broker accept the message. The PubRel and PubComp do not affect the broker reception of message, but some way to let broker release QoS2 message.

The original code by @de-sh notifies on PubComp not on PubRec for QoS2 which I believe to be the correct behavior. Because when using this feature, I want to be notified once I'm certain the message will fulfil it's QoS. For QoS2, the message is only sent after sending PubRel to the broker, which the broker then acknowledges by responding with PubComp. If the client dies after receiving PubRec but before the broker received PubRel, the message might get lost and the NoticeFuture shouldn't resolve for a message that might not fulfil it's QoS, which for 1 and 2 means it not getting lost.

At least that's my take based on my understanding of how MQTT QoS works and my take on the NoticeFuture feature.

What I mean is we can get notice on message ownership transfer, which is mentioned in the section 4.4, "When its original sender receives the PUBREC packet, ownership of the Application Message is transferred to the receiver". But I am totally fine to be noticed by PUBCOMP, where message ownership definitely transferred.

BTW I see your works on migrating this ack wait feature to main in #883, hoping the PR and discuss here can help.

What I mean is we can get notice on message ownership transfer, which is mentioned in the section 4.4

Yeah, but that is transferring it to the broker, right? My understanding was what is described in Method A in this non-normative example in the spec: https://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Figure_4.3_%E2%80%93 In that case the message is only delivered onwards after the receiver has received PUBREL. (although for Method B it sends it with the PUBLISH already, so I'm kind of confused)

BTW I see your works on migrating this ack wait feature to main in #883, hoping the PR and discuss here can help.

Yeah, I was looking at this PR for reference, but basically wanted to preserve the original commits for easier rebasing/updating with main and making it easier to diff to main. Also there were a few more commits in the acked branch since you made this PR, those are also included there.

…ed_test

commit 518773d Author: CQ Xiao <[email protected]> Date: Tue May 21 14:47:32 2024 +0800 rumqttc: resume session only if CONNACK with session present 1 (bytebeamio#864) * Check if session present to restore pending publishes. * Modify changelog. * remove changes that don't seem to be related * refactor: improve readability * feat: apply changes to v4 * Remove session_expiry_interval related code. * test: set clean session * test: broker saved session * test: fix resume reconnect --------- Co-authored-by: Devdutt Shenoi <[email protected]> Co-authored-by: Devdutt Shenoi <[email protected]> commit 67d9ca7 Author: CQ Xiao <[email protected]> Date: Thu May 16 23:05:10 2024 +0800 feat(rumqttc): set `session_expiry_interval` in v5 (bytebeamio#854) Co-authored-by: Devdutt Shenoi <[email protected]> commit 98997d1 Author: CQ Xiao <[email protected]> Date: Wed Jul 3 18:13:54 2024 +0800 HashMap -> VecDeque, cleanup

xiaocq2001 · 2024-07-04T02:53:12Z

Some updates for reference:

Sync with latest acked
HashMap -> VecDeque
- To save memory
- VecDeque is fast to push_back and pop_first
Publish NoticeTx
Notify on QoS2 PUBCOMP
Improve example to delay a while to see all logs

KillingJacky · 2024-09-25T06:21:06Z

Hi @xiaocq2001 , thanks for your work, this feature is exactly what I need for my current project. I can't wait for the upstream, and your PR seems quite promising, so how mature is your PR? If I can spend some effort to do the tests, can I use your PR right now?

xiaocq2001 · 2024-09-26T02:23:31Z

Hi @xiaocq2001 , thanks for your work, this feature is exactly what I need for my current project. I can't wait for the upstream, and your PR seems quite promising, so how mature is your PR? If I can spend some effort to do the tests, can I use your PR right now?

Feel free to test and use that.

de-sh

Please provide your suggestions against #916

de-sh · 2024-09-29T18:35:28Z

rumqttc/CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * `size()` method on `Packet` calculates size once serialized.
 * `read()` and `write()` methods on `Packet`.
 * `ConnectionAborted` variant on `StateError` type to denote abrupt end to a connection
+* `set_session_expiry_interval` and `session_expiry_interval` methods on `MqttOptions`.


Sorry for the extremely late response, if you are still interested in contributing this change, please do open a separate PR. Currently this PR will be hard to accept given it has deviated from the issue focus.

For session expiry interval support, it's already done in #854.

KillingJacky · 2024-09-30T05:52:08Z

Hi @xiaocq2001 , thanks for your work, this feature is exactly what I need for my current project. I can't wait for the upstream, and your PR seems quite promising, so how mature is your PR? If I can spend some effort to do the tests, can I use your PR right now?

Feel free to test and use that.

I'v done integrating with your acked_test branch and tested it with several test cases, so far so good, thank you very much for your work.

ryanwinter · 2024-10-29T17:30:13Z

Thanks @KillingJacky!

Devdutt Shenoi and others added 5 commits April 7, 2024 23:28

fix: v5 doesn't write outgoing packets onto network (bytebeamio#842)

d4c30df

* fix: v5 doesn't write outgoing packets onto network bytebeamio#825 (comment) * same for pingreq

chore(deps): bump h2 from 0.4.2 to 0.4.4 (bytebeamio#839)

f72acc8

Merge branch 'main' of https://github.com/bytebeamio/rumqtt into acke…

f641bc0

…d_test

Add error code for V5 PubAck/Rec/Comp, improve Notify flow

e191890

Remove pkid things, modify change log.

4cb80e1

xiaocq2001 mentioned this pull request Apr 26, 2024

RFC(rumqttc): publish / subscribe / unsubscribe methods return a promise that resolves into pkid when packet is handled by Eventloop #805

Open

xiaocq2001 changed the title ~~Acked POC improvements~~ publish / subscribe / unsubscribe methods return a notice future that waits publish (QoS0), PubAck (QoS1) and PubComp (QoS2) May 6, 2024

xiaocq2001 marked this pull request as ready for review May 14, 2024 01:39

xiaocq2001 added 3 commits May 23, 2024 14:37

Fix testing issue.

f46920a

Let user notified on incoming pubrec.

8005ce4

Remove Notify for PUBREL.

8774ed2

xiaocq2001 changed the title ~~publish / subscribe / unsubscribe methods return a notice future that waits publish (QoS0), PubAck (QoS1) and PubComp (QoS2)~~ publish / subscribe / unsubscribe methods return a notice future that waits publish (QoS0), PubAck (QoS1) and PubRec (QoS2) Jun 3, 2024

Notice wait drop due to session reset.

07c25b0

FSMaxB reviewed Jun 19, 2024

View reviewed changes

xiaocq2001 added 5 commits July 3, 2024 10:49

Merge branch 'acked' of https://github.com/bytebeamio/rumqtt into ack…

7da21b6

…ed_test

Update tests.

576bd79

Publish NoticeTx.

a63216f

Notify on QoS2 pubcomp

fb290fb

de-sh reviewed Sep 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

publish / subscribe / unsubscribe methods return a notice future that waits publish (QoS0), PubAck (QoS1) and PubRec (QoS2) #851

publish / subscribe / unsubscribe methods return a notice future that waits publish (QoS0), PubAck (QoS1) and PubRec (QoS2) #851

xiaocq2001 commented Apr 26, 2024 •

edited

Loading

FSMaxB left a comment

FSMaxB Jun 19, 2024

xiaocq2001 Jun 20, 2024

FSMaxB Jun 20, 2024

xiaocq2001 Jun 21, 2024

xiaocq2001 Jun 21, 2024 •

edited

Loading

FSMaxB Jun 21, 2024

xiaocq2001 commented Jul 4, 2024 •

edited

Loading

KillingJacky commented Sep 25, 2024

xiaocq2001 commented Sep 26, 2024

de-sh left a comment •

edited

Loading

de-sh Sep 29, 2024

xiaocq2001 Sep 30, 2024

KillingJacky commented Sep 30, 2024

ryanwinter commented Oct 29, 2024

publish / subscribe / unsubscribe methods return a notice future that waits publish (QoS0), PubAck (QoS1) and PubRec (QoS2) #851

Are you sure you want to change the base?

publish / subscribe / unsubscribe methods return a notice future that waits publish (QoS0), PubAck (QoS1) and PubRec (QoS2) #851

Conversation

xiaocq2001 commented Apr 26, 2024 • edited Loading

Type of change

Checklist:

FSMaxB left a comment

Choose a reason for hiding this comment

FSMaxB Jun 19, 2024

Choose a reason for hiding this comment

xiaocq2001 Jun 20, 2024

Choose a reason for hiding this comment

FSMaxB Jun 20, 2024

Choose a reason for hiding this comment

xiaocq2001 Jun 21, 2024

Choose a reason for hiding this comment

xiaocq2001 Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

FSMaxB Jun 21, 2024

Choose a reason for hiding this comment

xiaocq2001 commented Jul 4, 2024 • edited Loading

KillingJacky commented Sep 25, 2024

xiaocq2001 commented Sep 26, 2024

de-sh left a comment • edited Loading

Choose a reason for hiding this comment

de-sh Sep 29, 2024

Choose a reason for hiding this comment

xiaocq2001 Sep 30, 2024

Choose a reason for hiding this comment

KillingJacky commented Sep 30, 2024

ryanwinter commented Oct 29, 2024

xiaocq2001 commented Apr 26, 2024 •

edited

Loading

xiaocq2001 Jun 21, 2024 •

edited

Loading

xiaocq2001 commented Jul 4, 2024 •

edited

Loading

de-sh left a comment •

edited

Loading