eclipse / paho.mqtt.python

paho.mqtt.python
Other
2.15k stars 724 forks source link

application controlled acknowledgements to match Java behaviour #753

Closed petersilva closed 7 months ago

petersilva commented 10 months ago

The existing python library acknowledges all messages received after calling the on_message() callback configured by the application. This PR adds manual_acks=False as an argument to client.py/init() function. When manual_acks=True, the application must acknowledge receipt of every message using the newly created ack(mid,qos) entry point.

This PR addresses issue #348 to allow applications to delay acknowledgement until it has completely received it. There was a first PR (#554) submitted two years ago. and got some great feeback from it:

After much too and fro, it was fairly clear that the library cannot be responsible for ordering. Then it occurred to me, that the Java implementation exists, and the equivalent functionality has been present for a number of years, and so chances are that it is doing "the right thing" tm.

So this PR uses the java implementation as a model, and slavishly implements the same thing in Python.

To illustrate the relationship to Java implementation, we can compare some tidbits:

So in python we have:


in _handle_publish(self):
     .
     .
     .
      elif message.qos == 1:
            self._handle_on_message(message)
            if self._manual_ack:
                return MQTT_ERR_SUCCESS
            else:
                return self._send_puback(message.mid)

vs. Java:


in:     private void handleMessage(MqttPublish publishMessage)

                  // If we are not in manual ACK mode:
                if (!this.manualAcks && publishMessage.getMessage().getQos() == 1) {
                        this.clientComms.internalSend(new MqttPubAck(MqttReturnCode.RETURN_CODE_SUCCESS,
                                        publishMessage.getMessageId(), new MqttProperties()),
                                        new MqttToken(clientComms.getClient().getClientId()));
                }

There is a need for an implementation of a call to be used by the library user to actually send the acknowledgements. we can compare code here also:

python:

    def ack( self, mid, qos ):
        """
           send an acknowledgement for a given message id. (stored in message.mid )
           only useful in QoS=1 and auto_ack=False
        """
        if self._manual_ack :
            if qos == 1:
                return self._send_puback(mid)
            elif qos == 2:
                return self._send_pubcomp(mid)

        return MQTT_ERR_SUCCESS

versus Java:


       public void messageArrivedComplete(int messageId, int qos) throws MqttException {
                if (qos == 1) {
                        this.clientComms.internalSend(
                                        new MqttPubAck(MqttReturnCode.RETURN_CODE_SUCCESS, messageId, new MqttProperties()),
                                        new MqttToken(clientComms.getClient().getClientId()));
                } else if (qos == 2) {
                        this.clientComms.deliveryComplete(messageId);
                        MqttPubComp pubComp = new MqttPubComp(MqttReturnCode.RETURN_CODE_SUCCESS, messageId, new MqttProperties());
                        // @TRACE 723=Creating MqttPubComp due to manual ACK: {0}
                        log.info(CLASS_NAME, "messageArrivedComplete", "723", new Object[] { pubComp.toString() });

                        this.clientComms.internalSend(pubComp, new MqttToken(clientComms.getClient().getClientId()));
                }
        }

In the java, there was an existing messageArrivedCompleted entry point to adjust. In python, nothing similar was obvious, so modified _handrel which would only apply in QoS==2... to suppress sending of pubcomp when manual_acks is active.

petersilva commented 9 months ago

@ralight not trying to be pushy... but you were very helpful with my last PR (#554) I thought a mild poke might be reasonable ?

petersilva commented 9 months ago

@aks @cclauss this PR is actually one I care about... nobody seems to be looking at it. I added the closes magic phrase once I learned it from the other PR.

petersilva commented 9 months ago

@BBBSnowball @HaraldGustafsson Does this look like it solves the problem? Perhaps try out the branch and confirm if it works for you?

akx commented 7 months ago

@PierreF Ping – you recently closed #348, mind taking a look at this?

petersilva commented 7 months ago

all good improvments. Thanks @PierreF !

PierreF commented 7 months ago

Thank for your contribution.

petersilva commented 7 months ago

Thanks! I think a lot of people are helped by this new function. I know committters on this repo are overloaded, but it isn't obvious how to help. If committers want some help, I would be willing, but I don't see how to get sufficient permission to be helpful.