Skip to content

Conversation

@benoitvidis
Copy link
Contributor

@benoitvidis benoitvidis commented Aug 1, 2019

What does this PR do?

When the jwt expires, all subscriptions are reset. Users need to manually resubscribe on the tokenExpired event.

This PR attempts to fix the following situations:

  • If we resubscribe on tokenExpired and autoResubscribe is on and if Kuzzle client gets disconnected and the token expires during the disconnection, when the client is reconnected, the subscriptions are added twice.
  • Some tokenExpired collisions can mess the attached events
  • UnhandledRejection issued by a failing autoresubscribe if the token expired in the meantime

Changes

  • The realtime controller cleans up the rooms on disconnect and networkError events to avoid some leaks.
  • The tokenExpired event is now handled at Kuzzle level and
    1. removes all subscriptions even if coming from the protocol layer
    2. throttles to avoid duplicates
  • Failing auto-re-subscriptions are now caught and trigger a discarded event.

@benoitvidis benoitvidis self-assigned this Aug 1, 2019
@benoitvidis benoitvidis added the wip label Aug 1, 2019
@Aschen Aschen changed the title fix duplicate subscriptions on reconnect Fix duplicate subscriptions on reconnect Aug 2, 2019
@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #437 into master will decrease coverage by 0.08%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
- Coverage   96.15%   96.07%   -0.09%     
==========================================
  Files          33       33              
  Lines        1612     1630      +18     
==========================================
+ Hits         1550     1566      +16     
- Misses         62       64       +2
Impacted Files Coverage Δ
src/controllers/realtime/room.js 100% <100%> (ø) ⬆️
src/Kuzzle.js 94.44% <86.95%> (-0.16%) ⬇️
src/controllers/realtime/index.js 98.41% <94.44%> (-1.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 080dd63...63965b8. Read the comment docs.

@benoitvidis
Copy link
Contributor Author

Keeping in wip for the time being:

  • some tests are missing
  • This snippet looses its subscriptions randomly after the token expires.

@benoitvidis benoitvidis force-pushed the kzl-1324-fix-duplicate-subs-on-reconnect branch from 3f445cb to 63965b8 Compare August 7, 2019 13:27
@benoitvidis benoitvidis removed the wip label Aug 7, 2019
@benoitvidis
Copy link
Contributor Author

Removing wip: The attached gist failure is due to a bug in Kuzzle and addressed here

Copy link
Contributor

@scottinet scottinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either target 6-dev or bump version numbers+explain why this is a hotfix.

@benoitvidis benoitvidis changed the base branch from master to 6-dev August 8, 2019 13:12
@benoitvidis
Copy link
Contributor Author

@scottinet You're right. The fixed behaviours have been there for a while now. I think we can wait for the next release.

@alexandrebouthinon alexandrebouthinon merged commit 871b3b0 into 6-dev Aug 9, 2019
@alexandrebouthinon alexandrebouthinon deleted the kzl-1324-fix-duplicate-subs-on-reconnect branch August 9, 2019 16:13
@scottinet scottinet mentioned this pull request Sep 11, 2019
scottinet added a commit that referenced this pull request Sep 11, 2019
# [6.2.2](https://github.com/kuzzleio/sdk-javascript/releases/tag/6.2.2) (2019-09-11)


#### Bug fixes

- [ [#437](#437) ] Fix duplicate subscriptions on reconnect   ([benoitvidis](https://github.com/benoitvidis))

#### Enhancements

- [ [#439](#439) ] Add timeout support to Http protocol   ([Aschen](https://github.com/Aschen))
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants