refactor(rumqttc): Replace Vec with FixedBitSet for QoS 2 packet trac…

[hippalus]

…king.(#868)

Type of change

• Refactor (code improvements and optimizations)
• Replaced Vec<Option<u16>> with FixedBitSet for outgoing_rel and incoming_pub.
• Updated methods to accommodate the new data structure, including clean, handle_incoming_pubrec, handle_incoming_pubrel, and others.
• Updated unit tests to ensure correct functionality with the new data structure.
  refactor(mqtt_state): Replace Vec with FixedBitSet for QoS 2 packet tracking (rumqttc: Reduce memory usage of MqttState #868)

Refactored the MqttState struct to use FixedBitSet instead of Vec<Option<u16>> for the outgoing_rel and incoming_pub fields. This change optimizes memory usage and improves performance for tracking packet identifiers in QoS 2 operations.

Before

After

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.

[de-sh]

Thanks for the PR, these are some really good changes, would appreciate if you could undo the unrelated ones and apply similar updates to v5/state.rs. Please update CHANGELOG.md as well.

[de-sh]

This change seems to be unrelated to the core of this PR

[de-sh]

same as above

[de-sh]

We have placed things this way to make it clear where that particular error is trickling down from.

[coveralls]

Pull Request Test Coverage Report for Build 9192788128

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 26 of 35 (74.29%) changed or added relevant lines in 2 files are covered.
• 97 unchanged lines in 4 files lost coverage.
• Overall coverage decreased (-0.07%) to 36.133%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
rumqttc/src/state.rs	15	18	83.33%
rumqttc/src/v5/state.rs	11	17	64.71%

Files with Coverage Reduction	New Missed Lines	%
rumqttc/src/state.rs	3	78.37%
rumqttc/src/v5/state.rs	5	67.09%
rumqttc/src/eventloop.rs	29	83.85%
rumqttc/src/v5/eventloop.rs	60	8.65%

Totals
Change from base Build 9170133164:	-0.07%
Covered Lines:	5975
Relevant Lines:	16536

---

💛 - Coveralls

[de-sh]

LGTM
Great first PR @hippalus! 🎊

edit: sorry, forgot to mention that we need an update to the rumqttc/CHANGELOG.md file, e.g.

* Use `FixedBitSet` instead of `Vec<Option<u16>>` to improve memory utilization.

[Arend-Jan]

now that is what I call an improvement. Thanks @hippalus 🙏

[xiaocq2001]

@hippalus

Reduce the pkid to fixed bit seems work, but it assumes the pkids from host is within 1 to max inflight. It seems in MQTT spec there is no rule to limit packet identifiers in this way.
I think there could be issue to handle if a broker sending randomized or very large pkids, or scattered pkids, e.g., the bit to pkid value issue, the pkid actual ordering issue.

[de-sh]

I think there could be issue to handle if a broker sending randomized or very large pkids, or scattered pkids, e.g., the bit to pkid value issue, the pkid actual ordering issue.

Is this with regards to v4/5?

[swanandx]

Is this with regards to v4/5?

both. I think what @xiaocq2001 wants to say is let's say we have max inflight set to 100, but nothing is stopping broker from using 200 as pkid right?

so now when we get incoming pub with pkid 200, we will try to insert it and crash! please correct me if wrong.

btw, if this is the issue, then i think it was still there before this PR.

[hippalus]

Thank you, @xiaocq2001, @de-sh, and @swanandx, for the feedback.

You are correct about the potential issue with pkids beyond the assumed range. As @swanandx mentioned, the previous implementation depended on a limited range.

To address this, I've updated the implementation locally to use FixedBitSet::with_capacity(u16::MAX as usize + 1) for both outgoing_rel and incoming_pub. This ensures we handle the full range of pkids (1 to 65536) as per the MQTT spec. All test cases now pass with this change, with just an 8 KB memory increase.

It would be nice to add resiliency tests for handling large, randomized, or out-of-order pkids from the broker.

[xiaocq2001]

There is another issue, while re-sending the PUBREL packets, as described in MQTTv5 spec, "The Client MUST send PUBREC packets in the order in which the corresponding PUBLISH packets were received (QoS 2 messages) [MQTT-4.6.0-3]", this means if a connection is resumed the previous order of outgoing PUBREL must be restored, but FixedBitSet of outgoing_ref does not support ordering other than continuous and increasing pkid.

E.g., if broker sending publish packets with pkids 4,3,2,1 the expected outgoing_ref should be 4,3,2,1, but currently if it's disconnected and reconnected before the outgoing_ref sent, the resumed packet ordering is 1,2,3,4 which is not expected.

Another consideration is, for outgoing PUBLISH and PUBREL packets, they may need more information for status notification (#805), in such cases we must use larger structs instead of just one bit for each packet.