Fix for unlimited maximum message expiry interval

[dadebue]

fixes #306

[mochi-co]

I will try to look at this today, apologies for the delay

[coveralls]

Pull Request Test Coverage Report for Build 6541851124

• 26 of 26 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.004%) to 98.912%

---

Totals
Change from base Build 6536886378:	0.004%
Covered Lines:	5455
Relevant Lines:	5515

---

💛 - Coveralls

[mochi-co]

@werbenhu The current changes on the branch don't fully comply with the scenarios in #315 (comment) - With s.Options.Capabilities.MaximumMessageExpiryInterval == math.MaxInt64 we should still continue to expire messages which have their own set expiry values set.

I'd recommend reviewing the diff I posted above as I believe this addresses all the scenarios we are trying to fix.

[werbenhu]

I'm just catching up on this and it sounds like there are some misunderstandings on the spec and current design, but to confirm:

• Setting s.Options.Capabilities.MaximumMessageExpiryInterval == math.MaxInt64 should not disable message expiry, it should be honoured even though it is infinitely in the future - unless there is a significant performance reason not to do so (I am unaware of such a thing currently).
• if s.Options.Capabilities.MaximumMessageExpiryInterval is 1 or more, message expiry per message must still be honoured per the MQTTv5 spec if set in the message properties, but MQTTv3 messages and those without expiries should be retained forever.
• if s.Options.Capabilities.MaximumMessageExpiryInterval is 0, message expiry per message must be ignored for all messages and protocols.

(edited - apologies it's late and I've been extraordinarily busy)

It's done now.

[werbenhu]

There still seem to be some issues here, I'll need to make some submissions later

[mochi-co]

There still seem to be some issues here, I'll need to make some submissions later

Which issues are you seeing?

[werbenhu]

I still think that we shouldn't disable the clearing of expired retained messages if s.Options.Capabilities.MaximumMessageExpiryInterval == math.MaxInt64 ``

What would be the result if a sender sends a retained message that has a expiry of 1 hour but s.Options.Capabilities.MaximumMessageExpiryInterval == math.MaxInt64? The message will not be deleted from the broker because clearing of retained messages is disabled and therefore will not expire in 1 hour as the sender intended...

What do you think?

@mochi-co I think @dadebue is right, and I've made some changes to it.

[werbenhu]

@mochi-co take a look at the current implementation. I always feel that what's written here in func (cl *Client) ClearInflights(now, maximumExpiry int64) []uint16 isn't concise and clear enough;

'maximumExpiry=0' carries a dual meaning:

• When calling cl.ClearInflights() within server.clearExpiredInflights, 'maximumExpiry=0' means there's no need to process the maximum.
• When calling cl.ClearInflights() within server.inheritClientSession, 'maximumExpiry=0' indicates the need to forcibly clear all client inflights. Therefore, here I use maximumExpiry=-1 to denote a forced clear.

[dadebue]

I like the current implementation.

Maybe we should change the name abandoned to something different. What the maximum expiry interval is doing is forcing the expiry of old messages. Why don't we call it forceExpiry then?

[werbenhu]

@dadebue @mochi-co I have added client.ClearExpiredInflights() to clear expired messages, while client.ClearInflights() is used to clear all inflight messages. Please review the code again. Thanks.

[dadebue]

I approve the changes with this comment ☺️ (I can't approve in GitHub as I wasn't requested to review)

[werbenhu]

@dadebue Because you are the author of this PR.:smile:

[mochi-co]

It took a bit of a confusing road to get here, but I've given it a thorough review and it makes sense to me, and I think this is an excellent solution which makes the overall codebase a bit cleaner and easier to understand. Paho and unit tests are also passing, so I approve! Great job @werbenhu @dadebue

[dadebue]

@mochi-co @dgduncan @werbenhu what's is the release schedule for this repo? When will all the changes since October 2nd be released?

[mochi-co]

@dadebue Apologies, I've had some big life stuff going on and the time got away from me. I've released these changes as v2.4.2 :)

[dadebue]

No worries and thanks :)