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

Replay and retained message logic need work (biggest issue: inactive client sessions grow RAM forever) #237

Open
shipmints opened this issue Jan 19, 2021 · 0 comments

Comments

@shipmints
Copy link
Contributor

Session Message Replay Issues

This is a spot that is broken and currently (and correctly) disabled for qos 0 messages. See https://www.hivemq.com/blog/mqtt-essentials-part-7-persistent-session-queuing-messages/ for persistent session discussion where only QOS 1/2 should be retained for session-reconnect replay.

Here when a client has an active connection to the broker, it will be sent the pending message:

https://github.com/beerfactory/hbmqtt/blob/07c4c70f061003f208ad7e510b6e8fce4d7b3a6c/hbmqtt/broker.py#L709

When an old client session is recorded as having disconnected, every single message is enqueued waiting for the client to reconnect. If the client never reconnects, memory is consumed forever.

https://github.com/beerfactory/hbmqtt/blob/07c4c70f061003f208ad7e510b6e8fce4d7b3a6c/hbmqtt/broker.py#L719-L726

  • The logic assumes that when the client reconnects (assuming it's the same client by client id), it will be a dirty connection and will want the same subscriptions as it did the first time. I feel this is erroneous as one can only know the client's intentions after it has established its subscriptions for a clean session or stated clearly that it wants a non-clean, reused session.
  • The logic should not bother to retain session replay messages for anonymous client sessions. This is a related bug: If a client does not specify a client id, a random UUID is generated and that is stored forever in the broker session cache. This id (and session) will never be reused by the same client which will always send empty client id forcing a new random id to be created. The unreachable session will collect messages for a client connection that can never be reestablished, further exhausting memory.
  • Only messages with QOS 1 or QOS 2 should be put into this queue (hence the QOS 0 suppression in the current patched logic).
  • Configurable caps could be available to control the broker's behavior for the retained message queue either by: total bytes, number of messages, number of topics, list of specific topics.
  • The session collection of these messages probably should not be named retained_messages since its confusing. These are for high QOS guaranteed-delivery attempts.

Retained Topic Message Issues

Persistent topic subscription restoration looks like it needs work to ensure retained messages are provided when a non-clean session is restored the list of session topics is not reprocessed through the retained topic replay logic as it does when a new subscription is made.

Client session retrieved from cache but does not correctly reestablish topic subscriptions and will miss retained messages for those topics upon reconnection:

https://github.com/beerfactory/hbmqtt/blob/07c4c70f061003f208ad7e510b6e8fce4d7b3a6c/hbmqtt/broker.py#L381-L387

Here is where the logic for replaying the retained topic cache for new subscriptions is that should be reused for sessions pulled from the session cache:

https://github.com/beerfactory/hbmqtt/blob/07c4c70f061003f208ad7e510b6e8fce4d7b3a6c/hbmqtt/broker.py#L473-L480

Configurable caps could be available to control the broker's behavior for the topic retained messages similar to those proposed for the session message cache, above.

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

No branches or pull requests

1 participant