bytebeamio / rumqtt

The MQTT ecosystem in rust
Apache License 2.0
1.59k stars 247 forks source link

Unexpected Reconnect on MqttState errors #875

Closed vaavva closed 3 months ago

vaavva commented 3 months ago

Expected Behavior

UnsubFail/PubAckFail/etc should not act as a disconnect as they are packet specific and fully recoverable.

Current Behavior

In the event loop, anything that is an Err, not Ok calls self.clean() https://github.com/bytebeamio/rumqtt/blob/main/rumqttc/src/v5/eventloop.rs#L168, which should only be called when the client is disconnected (https://github.com/bytebeamio/rumqtt/blob/main/rumqttc/src/v5/eventloop.rs#L120)

However, ConnectionError includes MqttState errors (https://github.com/bytebeamio/rumqtt/blob/main/rumqttc/src/v5/eventloop.rs#L38), which includes things like EmptySubscription, OutgoingPacketTooLarge, UnsubFail, PubAckFail, etc https://github.com/bytebeamio/rumqtt/blob/main/rumqttc/src/v5/state.rs#L38 . So any of these errors will act as a disconnect it seems.

Failure Information (for bugs)

Please help provide information about the failure if this is a bug. If it is not a bug, please remove the rest of this template.

Context

Please provide any relevant information about your setup. This is important in case the issue is not reproducible except for under certain conditions.

Failure Logs

On duplicate unsubs (second should return a failure rc with NoSubscriptionExisted), which is shown from the event loop, but a reconnect also occurs

[MQTT] Outbound Unsubscribe: 3
[MQTT] Outbound Unsubscribe: 4
[MQTT] Incoming UNSUBACK: UnsubAck { pkid: 3, reasons: [Success], properties: None }
[MQTT] Unsub Fail: NoSubscriptionExisted
[MQTT] Connected!
[MQTT] Incoming CONNACK: ConnAck
...

mosquitto broker logs:

1717600422: Received UNSUBSCRIBE from sample_client
1717600422:     foo_topic/bar
1717600422: sample_client foo_topic/bar
1717600422: Sending UNSUBACK to sample_client
1717600422: Received UNSUBSCRIBE from sample_client
1717600422:     foo_topic/bar
1717600422: sample_client foo_topic/bar
1717600422: Sending UNSUBACK to sample_client
1717600422: Client sample_client closed its connection.
1717600422: New connection from 127.0.0.1:54500 on port 1884.

For a pub on an unauthorized topic

[APP] Publishing message 1
[MQTT] Error: MqttState(PubAckFail { reason: NotAuthorized })
[MQTT] Trying connect again in 5s
[MQTT] Connected!
...

mosquitto broker logs:

1717519140: Sending CONNACK to sample_client (0, 0)
1717519145: Denied PUBLISH from sample_client (d0, q1, r0, m1, 'send_topic/server', ... (18 bytes))
1717519145: Sending PUBACK to sample_client (m1, rc135)
1717519145: Client sample_client closed its connection.
1717519150: New connection from 127.0.0.1:42276 on port 1884.
swanandx commented 3 months ago

Thanks for reporting the issue. Yes, puback, unsuback / other acks shouldn't disconnect the client. Only some of the state errors should lead to disconnect.

Would love to see PR to fix this behaviour!

xiaocq2001 commented 3 months ago

Proposed a fix in https://github.com/bytebeamio/rumqtt/pull/877, please check.

swanandx commented 3 months ago

PR #877 is merged, can you please verify if it resolves the issue @vaavva ? thanks :)

vaavva commented 3 months ago

Yes, this fixed my issue, thank you!