Implement session expiry cleanup for persisted sessions #739

andsel · 2023-03-20T14:46:24Z

Release notes

Implement the logic to expire the not clean sessions.

What does it do?

Track all expiration time after a session is closed or disconnects. Uses a delay queue to track the not clean session that expires and using a scheduled task, every second, check for the expired sessions and removes from the both SessionRegistry and also from the persistent store (SessionRepositoy).

Why it's important for the user?

This feature let the user to auto-purge the state of the not clean session after a certain amount of time configured at broker level in moquette.conf file through the persistent_client_expiration property which could have the format:
<number><unit> such as 1d for 1 day. Admitted values to unit are s m h d w M y which respectively means seconds, minutes, hours, days, weeks, months and years. If not specified this global expiry defaults to INFINITE_EXPIRY which is ~80 years.

close Add session timeout #662

andsel · 2023-03-29T11:55:41Z

Hi @hylkevds may I ask you if you would like to be my buddy in this effort and provide a review to this functionality, please 🙏 ?

hylkevds

Looks good, just one concern about concurrency and one about re-activated sessions not being removed from the expiration queue.

broker/src/main/java/io/moquette/broker/SessionRegistry.java

hylkevds · 2023-04-11T18:46:32Z

broker/src/main/java/io/moquette/broker/SessionRegistry.java

+        }
+    }
+
+    private void trackForRemovalOnExpiration(ISessionsRepository.SessionData session) {


I think we also need the opposite, unTrackForRemovalOnExpiration, for when a session is re-opened.

Hi @hylkevds, you are right. It has been fixed with ae786d0, so please could you give it another review?

Don't forget to implement it :)

hylkevds · 2023-05-28T07:28:33Z

broker/src/main/java/io/moquette/broker/SessionRegistry.java

+        }
+    }
+
+    private void trackForRemovalOnExpiration(ISessionsRepository.SessionData session) {


Don't forget to implement it :)

hylkevds · 2023-05-28T07:28:53Z

broker/src/main/java/io/moquette/broker/SessionRegistry.java

+    }
+
+    private void untrackFromRemovalOnExpiration(ISessionsRepository.SessionData session) {
+


Needs implementation...

Fixed by these 2 commits:

250752b fixes test condition to verify effectively as failure when there is no implementation of the method untrackFromRemovalOnExpiration

d2a5d3f implements the untracking method

hylkevds · 2023-05-28T07:38:42Z

broker/src/main/java/io/moquette/broker/config/IConfig.java

+        String propertyValue = getProperty(propertyName);
+        final char timeSpecifier = propertyValue.charAt(propertyValue.length() - 1);
+        final TemporalUnit periodType;
+        switch (timeSpecifier) {


Please also add seconds and minutes. In high-volume cases we don't want sessions to stay alive for more than a minute or so...

Fixed with e29ad31

hylkevds · 2023-05-30T08:44:47Z

While working on the subscription tree this weekend I noticed that subscriptions are not cleaned up when a session is cleaned up.
I noticed because the interceptor didn't get notified of any unsubscribe events when a (clean) session terminated due to a disconnect.

This may need its own issue, WDYT?

andsel · 2023-06-02T19:29:27Z

@hylkevds I think this could be a bug but:

interceptor didn't get notified of any unsubscribe events

the interceptor is notified only when an UNSUBSCRIBE message is received. When a clean session is wiped due to a disconnection it doesn't mean that the unsubscribe interceptor is notified, or do you thing it should?

hylkevds · 2023-06-02T20:55:10Z

I think it should, yes.
If an interceptor wants to know about subscribe and unsubscribe events it's most likely to keep track of which topics have subscribers so it know which topics to send data to. For that use-case it is important to get all unsubscribe events, regardless of why one happened.

In our specific use-case there is no simple one-to-one mapping between events and the topics that may be interested in those events. It's an (in some cases extreme) many-to-many mapping. One event may have to be sent to potentially thousands of topics. That's why we need to keep track of exactly which topics actually have subscribers, so we can avoid generating messages for topics that have no subscriptions.

Of course the subscription not being cleaned up and the interceptor not being notified are two distinct, but related, issues.

andsel · 2023-06-03T10:21:23Z

@hylkevds please open an issue describing for that, describing the behavior. It will addressed in a separate PR.

andsel · 2023-06-03T10:57:06Z

Hi @hylkevds I've addressed all your latest concerns. Please could you review it?! :-)

hylkevds

Looks good!

…for non clean sessions

- made SessionData opaque of time using a Clock so that time can be easily managed in unit tests - spread the use of this Clock over interested classes and introduced test utilty class ForwardableClock - introduced delete method of session into Session's repository and used from the cleanup expired method.

…rsistent_client_expiration

…e cleanUp of removed sessions

…e expiry date and not now

…tionComputed

…n tracking

…uldn't be purged

…expiry interval

Leverage the existing global expiry harness introduced with #739, reading the expiration time from the CONNECT's MQTT property SESSION_EXPIRY_INTERVAL.

andsel force-pushed the feature/session_expiry_cleanup branch 2 times, most recently from a2851ee to 85dd80b Compare March 26, 2023 16:08

andsel changed the title ~~[WIP] Session expiry cleanup~~ Implement session expiry cleanup for persisted sessions Mar 26, 2023

andsel added the v0.17.0 label Mar 26, 2023

andsel marked this pull request as ready for review March 29, 2023 10:59

andsel force-pushed the feature/session_expiry_cleanup branch from 7d40949 to cf36fd7 Compare March 29, 2023 11:02

hylkevds reviewed Apr 11, 2023

View reviewed changes

andsel force-pushed the feature/session_expiry_cleanup branch from cf36fd7 to 203edd4 Compare April 14, 2023 15:32

andsel force-pushed the feature/session_expiry_cleanup branch from c1acc2b to ad2bb43 Compare May 19, 2023 14:39

andsel requested a review from hylkevds May 20, 2023 13:25

hylkevds reviewed May 28, 2023

View reviewed changes

andsel requested a review from hylkevds June 3, 2023 10:56

hylkevds previously approved these changes Jun 3, 2023

View reviewed changes

andsel added 11 commits June 3, 2023 21:27

Pass the scheduler service to SessionRegistry

8366178

Creates cleaner task for expired sessions and update the expire date …

19dafe0

…for non clean sessions

Exposed property to customize the time a session can be persisted: pe…

d1dbca3

…rsistent_client_expiration

SessionRegistry takes the SessioEventLoopGroup and asynch executes th…

133eab8

…e cleanUp of removed sessions

Fixed bug, the Session's clock instant has to be used to calculate th…

f562065

…e expiry date and not now

Avoid integer overflow

1c94de5

Used global session expiry to 100 days in test

ea5ec45

Moved from static calculateExpiration method to instance's withExpira…

4021700

…tionComputed

On session reconnection it must be removed from the session expiratio…

bbfa96c

…n tracking

Fixed test condition in case of session reconnection, the session sho…

8067d24

…uldn't be purged

andsel added 2 commits June 3, 2023 21:36

Implemented the untrack session method

19be452

Added minutes and seconds as available time units to express session …

410b145

…expiry interval

andsel dismissed hylkevds’s stale review via 410b145 June 3, 2023 19:36

andsel force-pushed the feature/session_expiry_cleanup branch from e29ad31 to 410b145 Compare June 3, 2023 19:36

Added lin in changelog

3d728e7

andsel self-assigned this Jun 3, 2023

andsel merged commit b8bbc1e into moquette-io:main Jun 3, 2023

andsel mentioned this pull request Jun 17, 2023

Use session expiry from CONNECT message properties #753

Merged

andsel added a commit that referenced this pull request Jun 21, 2023

Use session expiry from CONNECT message properties (#753)

0cab6e6

Leverage the existing global expiry harness introduced with #739, reading the expiration time from the CONNECT's MQTT property SESSION_EXPIRY_INTERVAL.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement session expiry cleanup for persisted sessions #739

Implement session expiry cleanup for persisted sessions #739

andsel commented Mar 20, 2023 •

edited

Loading

andsel commented Mar 29, 2023

hylkevds left a comment

hylkevds Apr 11, 2023

andsel May 20, 2023

hylkevds May 28, 2023

hylkevds May 28, 2023

hylkevds May 28, 2023

andsel May 29, 2023

andsel Jun 3, 2023

hylkevds May 28, 2023 •

edited

Loading

andsel Jun 3, 2023

hylkevds commented May 30, 2023

andsel commented Jun 2, 2023

hylkevds commented Jun 2, 2023

andsel commented Jun 3, 2023

andsel commented Jun 3, 2023

hylkevds left a comment

		}

		private void untrackFromRemovalOnExpiration(ISessionsRepository.SessionData session) {

Implement session expiry cleanup for persisted sessions #739

Implement session expiry cleanup for persisted sessions #739

Conversation

andsel commented Mar 20, 2023 • edited Loading

Release notes

What does it do?

Why it's important for the user?

andsel commented Mar 29, 2023

hylkevds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hylkevds May 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hylkevds commented May 30, 2023

andsel commented Jun 2, 2023

hylkevds commented Jun 2, 2023

andsel commented Jun 3, 2023

andsel commented Jun 3, 2023

hylkevds left a comment

Choose a reason for hiding this comment

andsel commented Mar 20, 2023 •

edited

Loading

hylkevds May 28, 2023 •

edited

Loading