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

Clear existing timeout in initAutoRefresh #168

Closed
wants to merge 2 commits into from

Conversation

MaxDNG
Copy link

@MaxDNG MaxDNG commented Sep 18, 2024

What is this PR and why do we need it?

In React Strict mode, effects are run twice. This means that if the shouldAutoRefresh flag is on, the following effect will be triggered twice on first render:

useEffect(() => {
  if (shouldAutoRefresh) {
    initAutoRefresh();
  }
}, [initAutoRefresh, shouldAutoRefresh]);

In the SDK Core, the initAutoRefresh method will thus be called twice, setting two time-outs. However, only the last one will get tracked. When the FusionAuthProvider component gets re-rendered, it will create a new core, attempting to dispose() the old one. The dispose will successfully cancel the second timeout, but the first one is still running.

React initAutoRefresh 1
=> SDKCore initAutoRefresh sets this.refreshTokenTimeout (let's say to value 1)
React initAutoRefresh 2 (effect is re-run)
=> SDKCore initAutoRefresh sets this.refreshTokenTimeout (let's say to value 2)
=> refreshTimeout 1 is not cleared and keeps running
React FusionAuthProvider rerenders
=> SDKCore dispose method is called, clearing refreshTokenTimeout 2

Eventually refreshTimeout 1 will run and could potentially return a wrong value (in my case the clientId was not correct anymore)

Pre-Merge Checklist (if applicable)

  • Unit and Feature tests have been added/updated for logic changes, or there is a justifiable reason for not doing so.

@alex-fusionauth
Copy link
Contributor

I believe we have this address in #169, let me know if we do not and should reopen @MaxDNG.

Hit me up I would love to send you some swag!
https://x.com/codercatdev

@MaxDNG
Copy link
Author

MaxDNG commented Sep 23, 2024

@alex-fusionauth the issue fixed in #169 is slightly different. However, after installing 2.4.3 the fix I did here does not behave the same way.

I believe what I had is an edge-case: because I'm using multi-tenancy I don't know beforehand what's the clientId so I am mounting the FusionAuthProvider first without clientId to be able to use the onLogin method, then on redirect back to my app I have the clientId.
TL;DR: I managed to fix it on my side. If you want I can dig into the issue. From my first tests, it would need proper cleanup on unmount (with useEffect) which is not handled at the moment. But again I think this is an edge-case and 99.9% of user won't face it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants