Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle MessageExpiryInterval early enough for retained messages to honor it #442

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dariopb
Copy link

@dariopb dariopb commented Dec 19, 2024

Setting the pk.Expiry member to the right value early enough so it is correctly saved when the message is retained in RetainMessage.

Without the change, even if we set the MessageExpiryInterval for retained message, the message won't expire at that interval since it was stored already with the default value (0) for pk.Expiry.

@thedevop
Copy link
Collaborator

Thank you @dariopb for the PR. The identification of the issue is correct. In reviewing this, I found we're not providing consistent behavior when MaximumMessageExpiryInterval is set to 0. So we may need to make larger modifications.
@mochi-co, currently when MaximumMessageExpiryInterval is set to 0, sometimes the server behaves as

  1. no limit

    server/server.go

    Lines 1714 to 1717 in 8f52b89

    // If the maximum message expiry interval is set (greater than 0), and the message
    // retention period exceeds the maximum expiry, the message will be forcibly removed.
    enforced := s.Options.Capabilities.MaximumMessageExpiryInterval > 0 &&
    now-pk.Created > s.Options.Capabilities.MaximumMessageExpiryInterval
  2. yet sometimes it treats as expire immediately

    server/server.go

    Lines 989 to 992 in 8f52b89

    pk.Expiry = pk.Created + s.Options.Capabilities.MaximumMessageExpiryInterval
    if pk.Properties.MessageExpiryInterval > 0 {
    pk.Expiry = pk.Created + int64(pk.Properties.MessageExpiryInterval)
    }

Which meaning do you prefer?

@mochi-co
Copy link
Collaborator

mochi-co commented Jan 1, 2025

@thedevop I think if we are to look at it logically, then 0 should be expire immediately. We can always set an very high expiry (maxint), but we cannot otherwise set expire immediately (unless we set it to 1 and then there's a short delay).

If 1 expires after 1, then 0 should expire immediately I think. What do you think?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 12403881253

Details

  • 7 of 9 (77.78%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 98.498%

Changes Missing Coverage Covered Lines Changed/Added Lines %
server.go 7 9 77.78%
Files with Coverage Reduction New Missed Lines %
server.go 4 98.7%
Totals Coverage Status
Change from base Build 11487320372: -0.1%
Covered Lines: 6165
Relevant Lines: 6259

💛 - Coveralls

@thedevop
Copy link
Collaborator

thedevop commented Jan 1, 2025

Ok, so when MaximumMessageExpiryInterval is 0, it means expire immediately. We can use MaxInt to make it effectively unlimited if needed.

@dariopb, let me know if you can make the the corresponding changes in this PR or I can submit a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants