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

Add Request::PublishWithNotify to notify about selected pkid #240

Closed
wants to merge 1 commit into from

Conversation

mneumann
Copy link

  • This allows to get notified about the selected pkid for a given
    Publish message.

  • The purpose of this is be able to wait for PubAck (QoS=1) or PubComp
    (QoS=2) messages on a per message basis. For that we need to know the
    pkid that is choosen by rumqttc as we can't realiably provide our
    own pkid.

* This allows to get notified about the selected `pkid` for a given
  `Publish` message.

* The purpose of this is be able to wait for PubAck (QoS=1) or PubComp
  (QoS=2) messages on a per message basis. For that we need to know the
  `pkid` that is choosen by `rumqttc` as we can't realiably provide our
  own `pkid`.
@tekjar
Copy link
Contributor

tekjar commented Feb 7, 2021

How does this propagate all the way to client?

client.publish("a/b", QoS::AtleastOnce, false, "data").await?;

I don't see any publish method which uses PublishWithNotify

@mneumann
Copy link
Author

mneumann commented Feb 7, 2021

@tekjar We use the PublishWithNotify to associate a message with the pkid that is used by rumqttc. So, it would be easy to change the signature of publish to return the pkid instead of ().

We use this in our custom MQTT client (based on rumqttc) to implement a publish_with_ack method that allows you to wait for the Ack to arrive. Once you have the pkid, it is very easy to listen for the next incoming PubAck or (PubComp). Without knowing the pkid, the only thing you can do is to use the ordering of events returned from the rumqttc event loop and hope that the order exactly reflects the order of published messages (there are some corner cases, like collisions, that can destroy this order).

Ideally, rumqttc would fire on the oneshot queue once the PubAck or PubComp arrives. But that would be a larger change. If there is enough interest in having this functionality, and willingness to merge these changes back, I can work on implementing this.

@tekjar
Copy link
Contributor

tekjar commented Feb 7, 2021

Yeah. That would be good. This would be sync_publish and sync_subscribe in AsyncClient and Client.

@stale
Copy link

stale bot commented Mar 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Mar 16, 2022
@mladedav
Copy link
Contributor

As per MQTT 3.1.1:

[A client] MUST send PUBACK packets in the order in which the corresponding PUBLISH packets were received (QoS 1 messages) [MQTT-4.6.0-2]

A Server MUST by default treat each Topic as an "Ordered Topic". It MAY provide an administrative or other mechanism to allow one or more Topics to be treated as an "Unordered Topic" [MQTT-4.6.0-5].

They fail to define what an Ordered or Unordered Topic is but I assume that it means that the server may elect to acknowledge published messages in arbitrary order.

Is this something that can still be added to rumqttc or is the wontfix label correct?

@stale stale bot removed the wontfix This will not be worked on label Mar 19, 2022
@de-sh
Copy link
Member

de-sh commented Mar 19, 2022

@mladedav, the wontfix label was added by the bot. We had configured it wrong. It's supposed to be the "stale" label. Please consider it to mean something in the lines of "We are currently not seeing any activity on this issue/PR and hence we will be marking it as stale till there is an answer/commit in response by the author"

@@ -309,6 +310,17 @@ impl MqttState {
Ok(())
}

fn outgoing_publish_with_notify(&mut self, publish: Publish, notify: OneshotSender<u16>) -> Result<(), StateError> {
let () = self.outgoing_publish(publish)?;
if let Some(Event::Outgoing(Outgoing::Publish(pkid))) = self.events.back() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems dangerous in in parallel environments. There is no guarantee that this will happen (and therefore the client could wait indefinetly for the notification) and even if this happens, there is a chance that the pkid is wrong.

@mladedav
Copy link
Contributor

There is a different PR that addresses very similar issue in #291. When you say you cannot reliably provide pkid, can you at least provide semi-random unique correlation IDs?

@stale stale bot added the stale Not moving forward; blocked label Apr 10, 2022
@de-sh de-sh closed this Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Not moving forward; blocked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants