rumqttc: resume session only if CONNACK with session present 1

xiaocq2001 commented 1 month ago

Type of change

Implement for https://github.com/bytebeamio/rumqtt/issues/863.

Bug fix (non-breaking change which fixes an issue):

In MQTT, session is not always resumed, but in current mqttc the pending publishes are restored anyway. With this fix the CONNACK session present returned from broker is checked, to confirm if a session is actually resumed, if not the pending publishes are cleaned and new session is started.

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.

swanandx commented 1 month ago

can you please fix the code / tests?

the reconnection_resends_unacked_packets_from_the_previous_connection_first test is failing, i think we need to connect with clean session false so that broker will save the state right?

xiaocq2001 commented 1 month ago

To test, I think we need https://github.com/bytebeamio/rumqtt/pull/854. With that, we need to connect with clean session false and session expiry interval non-zero to let broker save state.

xiaocq2001 commented 1 month ago

To test, I think we need #854. With that, we need to connect with clean session false and session expiry interval non-zero to let broker save state.

Facing an issue on connection.

---- reconnection_resumes_from_the_previous_state stdout ----
Polled = Ok(Incoming(ConnAck(ConnAck { session_present: false, code: Success })))
Polled = Ok(Outgoing(Publish(1)))
Polled = Ok(Incoming(PubAck(PubAck { pkid: 1 })))
Polled = Ok(Outgoing(Publish(2)))
Polled = Ok(Incoming(PubAck(PubAck { pkid: 2 })))
Polled = Ok(Outgoing(Publish(3)))
Polled = Ok(Outgoing(Publish(4)))
Polled = Ok(Outgoing(Publish(5)))
Polled = Ok(Outgoing(PingReq))
Polled = Ok(Outgoing(Publish(6)))
Polled = Ok(Outgoing(Publish(7)))
Polled = Ok(Outgoing(Publish(8)))
Polled = Ok(Outgoing(Publish(9)))
Polled = Ok(Outgoing(Publish(10)))
Polled = Err(MqttState(AwaitPingResp))
Polled = Ok(Incoming(ConnAck(ConnAck { session_present: false, code: Success })))

It seems on host side session is not stored?

Another question: due to MQTT protocol difference between versions, in v5 session_expiry_interval is supported but in v3 it is not, what is the suggestion on handling such difference in tests?

de-sh commented 1 month ago

To test, I think we need #854. With that, we need to connect with clean session false and session expiry interval non-zero to let broker save state.

Facing an issue on connection.

---- reconnection_resumes_from_the_previous_state stdout ----
Polled = Ok(Incoming(ConnAck(ConnAck { session_present: false, code: Success })))
Polled = Ok(Outgoing(Publish(1)))
Polled = Ok(Incoming(PubAck(PubAck { pkid: 1 })))
Polled = Ok(Outgoing(Publish(2)))
Polled = Ok(Incoming(PubAck(PubAck { pkid: 2 })))
Polled = Ok(Outgoing(Publish(3)))
Polled = Ok(Outgoing(Publish(4)))
Polled = Ok(Outgoing(Publish(5)))
Polled = Ok(Outgoing(PingReq))
Polled = Ok(Outgoing(Publish(6)))
Polled = Ok(Outgoing(Publish(7)))
Polled = Ok(Outgoing(Publish(8)))
Polled = Ok(Outgoing(Publish(9)))
Polled = Ok(Outgoing(Publish(10)))
Polled = Err(MqttState(AwaitPingResp))
Polled = Ok(Incoming(ConnAck(ConnAck { session_present: false, code: Success })))

It seems on host side session is not stored?

https://github.com/xiaocq2001/rumqtt/pull/2 solves this inconsistency in the tests

Another question: due to MQTT protocol difference between versions, in v5 session_expiry_interval is supported but in v3 it is not, what is the suggestion on handling such difference in tests?

The tests focus mainly on v4 as of right now, we are yet to see contributions towards better testing the v5 code. But we are happy to...

xiaocq2001 commented 1 month ago

I realize that in failing tests, the broker is started by Broker::new, which starts a broker without any stored state, that's why broker send ConnAck with session present false, we need some new method to introduce disconnection, maybe like what is in async_manual_ack example.

OK. I saw the updates to update broker, that's great.

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 9166733626

Details

11 of 23 (47.83%) changed or added relevant lines in 2 files are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.002%) to 36.201%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rumqttc/src/v5/eventloop.rs	0	12	0.0%
<!--	Total:	11	23	47.83%	-->

Files with Coverage Reduction	New Missed Lines	%
rumqttc/src/v5/eventloop.rs	3	8.99%
<!--	Total:	3		-->

Totals
Change from base Build 9128770188:	0.002%
Covered Lines:	5987
Relevant Lines:	16538

bytebeamio / rumqtt