Skip to content

Conversation

@betocantu93
Copy link

@betocantu93 betocantu93 commented Mar 5, 2025

If you call setup twice, multiple event listeners are added: authenticationSucceeded invalidationSucceeded

This 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:

async safeSetup() {
    if (!this._setupIsCalled) {
      return this.setup();
    }
  }

fix: if setup is ran multiple times, multiple handlers were added
@betocantu93 betocantu93 changed the title Update session.ts session.setup must be guarded to avoid double calling Mar 5, 2025
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.
});
}
Copy link
Collaborator

@BobrImperator BobrImperator Mar 6, 2025

Choose a reason for hiding this comment

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

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.
});

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