session.setup must be guarded to avoid double calling#2951
Open
betocantu93 wants to merge 1 commit intomainmatter:masterfrom
Open
session.setup must be guarded to avoid double calling#2951betocantu93 wants to merge 1 commit intomainmatter:masterfrom
betocantu93 wants to merge 1 commit intomainmatter:masterfrom
Conversation
fix: if setup is ran multiple times, multiple handlers were added
Comment on lines
+337
to
+344
| if(!this._setupIsCalled) { | ||
| this._setupIsCalled = true; | ||
| this._setupHandlers(); | ||
|
|
||
| return this.session.restore().catch(() => { | ||
| // If it raises an error then it means that restore didn't find any restorable state. | ||
| }); | ||
| } |
Collaborator
There was a problem hiding this comment.
Thanks for the PR 👍
It seems like one alternative could be to use an instance-initializer instead (thinking out loud).
Could you add a test asserting that _setupHandlers can be called only once?
We might want to make sure that the setupHandlers runs only once while restore continues to be called.
Suggested change
| if(!this._setupIsCalled) { | |
| this._setupIsCalled = true; | |
| this._setupHandlers(); | |
| return this.session.restore().catch(() => { | |
| // If it raises an error then it means that restore didn't find any restorable state. | |
| }); | |
| } | |
| if (!this._setupIsCalled) { | |
| this._setupIsCalled = true; | |
| this._setupHandlers(); | |
| } | |
| return this.session.restore().catch(() => { | |
| // If it raises an error then it means that restore didn't find any restorable state. | |
| }); |
This file contains hidden or 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
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.
If you call setup twice, multiple event listeners are added:
authenticationSucceededinvalidationSucceededThis could happen easily if you place the setup call on any beforeModel, say application route beforeModel, without manual guarding. Any transition that happens in between while executing the beforeModel aborts the transition and triggers a new one, which causes the beforeModel to be triggered again, thus calling setup again. This prob should be guarded from within the lib.
As a work around we extended the service with: