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

Correctly identify and clean taken-over sessions #180

Merged
merged 1 commit into from
Feb 25, 2023

Conversation

mochi-co
Copy link
Collaborator

@mochi-co mochi-co commented Feb 25, 2023

An issue was highlighted in which the subscriptions for a client which has undertaken a session takeover could be cleared erroneously in #173.

This behaviour was caused by the orphaned client unsubscribing from the client's topics after the new client had already inherited the subscriptions and begun reading.

Additionally, I identified a secondary, more significant issue in which an expiring orphaned client would delete the existing client from the server client map.

This PR attempts to resolve this issue through three mechanisms:

  1. By adding a new isTakenOver atomic bit switch to the client which is set to 1 when the client session is inherited by a new client and the client is orphaned (the pointer to the client has been, or shortly will be overwritten in the server client map).
  2. The s.unsubscribeClient logic has been adjusted to operate in two steps: first, the client subscriptions are moved from the client itself. Then, unsubscribe from the server topics index is only performed if the client has not been taken over.
  3. When an actively connected client connection expires, it may only be deleted from the server clients map if the client has not been taken over (i.e. it is the active, operating client).

Finally, the state of existing sessions is cleaned up on all inheritance to prevent sequential take-overs from increasing memory usage by inflights + subs * client-id.

This PR adds a new test case specifically to ensure client inheritance and state cleaning is performed as expected.

@mochi-co mochi-co added the bug Something isn't working label Feb 25, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4267624673

  • 31 of 31 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 98.594%

Totals Coverage Status
Change from base Build 4247937237: 0.005%
Covered Lines: 5120
Relevant Lines: 5193

💛 - Coveralls

@mochi-co mochi-co merged commit 9b7a943 into master Feb 25, 2023
@mochi-co mochi-co deleted the identify-session-takeovers branch February 25, 2023 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants