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

persistent_client_expiration does not work with logrotate/reload #1494

Closed
ckrey opened this issue Nov 12, 2019 · 4 comments
Milestone

Comments

@ckrey
Copy link

@ckrey ckrey commented Nov 12, 2019

This problem is in MQTTv311:

When log is rotated and mosquitto gets reloaded, inactive clients are detected as disconnected. loop.coutputs the following message:

/var/log/mosquitto/mosquitto.log

...
1573532162: Client rkix disconnected.
...

loop.c calls context__disconnect which calls session_expiry__add which
sets the expiry for the inactive session to now + persistent_client_expiration.

If persistent_client_expiration is longer than one day, those clients will never expire...

/etc/mosquitto/mosquitto.conf

...
persistent_client_expiration 1w
...

/etc/logrotate.d/mosquitto

/var/log/mosquitto/mosquitto.log {
        rotate 7
        daily
        compress
        size 100k
        nocreate
        missingok
        postrotate
                if invoke-rc.d mosquitto status > /dev/null 2>&1; then \
                        invoke-rc.d mosquitto reload > /dev/null 2>&1; \
                fi;
        endscript
}
@ckrey

This comment has been minimized.

Copy link
Author

@ckrey ckrey commented Nov 12, 2019

As a result those inactive clients and possible a large number of queued messages are never purged from mosquitto (and mosquitto.db)

@ralight

This comment has been minimized.

Copy link
Contributor

@ralight ralight commented Nov 14, 2019

Other people have seen what looks like clients being disconnected on HUP, but I've never been able to reproduce it. Do you have any other mosquitto config that might be different to what I have?

@ckrey

This comment has been minimized.

Copy link
Author

@ckrey ckrey commented Nov 14, 2019

I cannot reproduce the problem of clients being disconnected on HUP using a stripped down test environment.

What I can reproduce is the fact that v311 clients never expire because for v311 clients session_expiry_interval is always set to UINT32_MAX on connect and the expiration check does
check for this value.

Probably a fix for the problem would be:

diff --git a/src/session_expiry.c b/src/session_expiry.c
index e720b703..7e182126 100644
--- a/src/session_expiry.c
+++ b/src/session_expiry.c
@@ -44,7 +44,7 @@ int session_expiry__add(struct mosquitto_db *db, struct mosquitto *context)
        item->context = context;
        item->context->session_expiry_time = time(NULL);
        if(db->config->persistent_client_expiration == 0 ||
-                       db->config->persistent_client_expiration < item->context->session_expiry_interval){
+                       db->config->persistent_client_expiration > item->context->session_expiry_interval){

                item->context->session_expiry_time += item->context->session_expiry_interval;
        }else{
@@ -95,8 +95,7 @@ void session_expiry__check(struct mosquitto_db *db, time_t now)
        last_check = now;

        DL_FOREACH_SAFE(expiry_list, item, tmp){
-               if(item->context->session_expiry_interval != UINT32_MAX
-                               && item->context->session_expiry_time < now){
+               if(item->context->session_expiry_time < now){

                        context = item->context;
                        session_expiry__remove(context);

P.S.
session_expiry__check does actually do the expiry, G_CLIENTS_EXPIRED_INC() in loop.c is never executed. You may want to include the stats and logging into session_expiry.c

@ralight

This comment has been minimized.

Copy link
Contributor

@ralight ralight commented Nov 14, 2019

Good point about the UINT32_MAX value. We have to keep that as a special value though which means "never expire" - except that the custom expiration must override it.

@ralight ralight added this to the 1.6.8 milestone Nov 28, 2019
ralight added a commit that referenced this issue Nov 28, 2019
Closes #1494. Thanks to Christoph Krey.
@ralight ralight closed this Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.