Added scheduled session cleanup #10802
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ore last session command processed
ViacheslavKlimov
requested changes
May 16, 2024
application/src/main/java/org/thingsboard/server/service/ws/DefaultWebSocketService.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/thingsboard/server/service/ws/DefaultWebSocketService.java
Outdated
Show resolved
Hide resolved
| @@ -539,6 +539,11 @@ public void close(WebSocketSessionRef sessionRef, CloseStatus reason) throws IOE | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean isOpen(String sessionId) { | |||
| return externalSessionMap.containsKey(sessionId); | |||
There was a problem hiding this comment.
I think that we should additionally check the actual native WS session:
sessionMd.session.isOpen().
@ashvayka Do you agree?
There was a problem hiding this comment.
I am not sure. If the session is removed from the map, it is no longer used.
ViacheslavKlimov
requested changes
May 16, 2024
| wsSessionsMap.values().forEach(md -> { | ||
| String sessionId = md.getSessionRef().getSessionId(); | ||
| if (!msgEndpoint.isOpen(sessionId)) { | ||
| log.info("Cleaning up stale session [sessionId: {}, tenantId: {}, userId: {}] ", sessionId, |
There was a problem hiding this comment.
Use standard way of logging: "[tenantId][userId][sessionId] Cleaning up stale session"
| md.getSessionRef().getSecurityCtx().getTenantId(), md.getSessionRef().getSecurityCtx().getId()); | ||
| wsSessionsMap.remove(sessionId); | ||
| oldSubService.cancelAllSessionSubscriptions(sessionId); | ||
| entityDataSubService.cancelAllSessionSubscriptions(sessionId); |
There was a problem hiding this comment.
Don't we need to also call processSessionClose(sessionRef)?
| @@ -107,6 +113,8 @@ public void initExecutor() { | |||
| subscriptionUpdateExecutor = ThingsBoardExecutors.newWorkStealingPool(20, getClass()); | |||
| tsCallBackExecutor = Executors.newSingleThreadExecutor(ThingsBoardThreadFactory.forName("ts-sub-callback")); | |||
| serviceId = serviceInfoProvider.getServiceId(); | |||
| staleSessionCleanupExecutor = Executors.newSingleThreadScheduledExecutor(ThingsBoardThreadFactory.forName("stale-session-cleanup")); | |||
| staleSessionCleanupExecutor.scheduleWithFixedDelay(this::cleanupStaleSessions, 0, 60000, TimeUnit.MILLISECONDS); | |||
There was a problem hiding this comment.
Initial delay - also 60 seconds
ashvayka
reviewed
May 17, 2024
| if (sessionSubscriptions.isEmpty()) { | ||
| subscriptionsBySessionId.remove(sessionId); | ||
| ModifySubscriptionResult modifySubscriptionResult = null; | ||
| subsLock.lock(); |
ashvayka
reviewed
May 17, 2024
| } | ||
|
|
||
| private void pushSubscriptionEvent(ModifySubscriptionResult modifySubResult) { | ||
| TbEntitySubEvent event = modifySubResult.getEvent(); |
There was a problem hiding this comment.
try catch with logging (warn) for both pushSubscriptionEvent and modifySubscription
ashvayka
reviewed
May 17, 2024
| private TbSubscription<?> missedUpdatesCandidate; | ||
| private TbEntitySubEvent event; | ||
|
|
||
| public boolean isEmpty() { |
| modifySubscription(subscription.getTenantId(), subscription.getEntityId(), subscription, false); | ||
| } catch (Exception e) { | ||
| log.warn("[{}][{}] Failed to remove subscription {} due to ", subscription.getTenantId(), subscription.getEntityId(), subscription, e); | ||
| List<ModifySubscriptionResult> modifySubscriptionResults = new ArrayList<>(); |
There was a problem hiding this comment.
-> SubscriptionModificationResult, for variable name might just use 'result(s)'
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request description
Added scheduled session cleanup for cases when session was closed before last session command processed
General checklist
Front-End feature checklist
Back-End feature checklist