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

Unhandled Promise error detected when using the manual session.setup function #2358

Closed
swelham opened this issue Jan 24, 2022 · 9 comments · Fixed by #2363
Closed

Unhandled Promise error detected when using the manual session.setup function #2358

swelham opened this issue Jan 24, 2022 · 9 comments · Fixed by #2363

Comments

@swelham
Copy link
Contributor

swelham commented Jan 24, 2022

We recently upgraded to v4 of ESA and noticed a deprecation warning. After searching through the repo I found the v4 upgrade guides and applied the changes suggested. When we rolled this out to production we immediately started getting Unhandled Promise error detected errors coming in on every page load.

I have verified this was due to the upgrade by redeploying with and without the this.session.setup(); call. From this, I can see the error is only produced when a call to setup is present. I have tried the two following approaches in our application route beforeModel function and both cases have resulted in the same error.

async beforeModel() {
  await this.session.setup();

  ... other code
}

and

beforeModel() {
  return this.session.setup().then(() => {
    ... other code
  });
}

Our ember version is 3.24.0 and ESA version is 4.1.1.

@BobrImperator
Copy link
Collaborator

BobrImperator commented Jan 24, 2022

Hello @swelham, can you make sure that useSessionSetupMethod is correctly setup?
Are you still using mixins by any chance?

Could you try debugging what is causing the rejection? If I'm not mistaken, when ESA fails to setup itself / restore authentication it doesn't raise an error.
The error most likely comes from non-authenticated requests on other routes instead.

Are there any requests failing when it occurs?

@swelham
Copy link
Contributor Author

swelham commented Jan 28, 2022

Hey @BobrImperator, I believe I have the useSessionSetupMethod correctly set. I have specified the following in our ember-cli-build.js file.

'ember-simple-auth': {
  useSessionSetupMethod: true,
},

I have spent some more time debugging and I have been able to capture the error coming from the internal-session module when restore is called. As you mentioned, this rejected promise is caught in a try..catch in the session module.

Unfortunately, our error tracking sdk (sentry) still catches the caught error and reports it. I can configure sentry to ignore the error, but as it's just a generic Unhandled Promise error we would end up filtering out all of these errors, potentially hiding a real bug in our app.

I am wondering if there is a way for the restore function to throw a more identifiable error in this case?

@BobrImperator
Copy link
Collaborator

The session.setup() shouldn't be returning any errors I believe, it already wraps anything that could potentially break and if they fail they do so silently.
You can try implementing your own try catch around the session.setup() in your application route and see if the error persists.

@swelham
Copy link
Contributor Author

swelham commented Feb 2, 2022

Thanks, I have dug a bit deeper and found that when the promise fails, the error is still raised in the browser but as a caught exception. After some experimenting, I worked out that using a .catch() on the promise would prevent this, meaning it doesn't get tracked by our error reporting.

For example, changing the restore part of the session setup function to something like this prevents the error from being raised.

await this.session.restore().catch(() => {
  // Ignore errors when restoring the session
});

Would this be an acceptable change?

@BobrImperator
Copy link
Collaborator

BobrImperator commented Feb 2, 2022

I think that's correct.

Though I'm not exactly sure how sentry manages to get this error even though it's already caught and hushed out internally by ESA.
It doesn't appear in browser console for me at all, meaning that it's not being thrown by Ember.onerror if I understand this bit of Ember correctly.
From the looks of it, it seems like sentry is getting the errors in some different way than hooking into Ember.onerror.

@swelham
Copy link
Contributor Author

swelham commented Feb 2, 2022

Yeah, I believe sentry is doing something lower level as I think it's more of a generic js SDK than ember specific. I also don't see the error in the console unless I enable break on all exceptions including caught exceptions.

@BobrImperator
Copy link
Collaborator

@swelham Could you let me know what version of sentry you're using and which package?

@swelham
Copy link
Contributor Author

swelham commented Feb 4, 2022

Sure, we're using "@sentry/browser": "^6.16.1" and "@sentry/integrations": "^6.16.1" in our app right now.

After some more debugging I noticed in the internal-session.restore function, where the reject is specified, there is no error passed into the call to reject (ie const reject = () => RSVP.Promise.reject(); vs const reject = () => RSVP.Promise.reject('some error');. I believe this function expects to receive an error reason when called and appears to have a different behaviour when it isn't given.

I tried the code, specifying a hard-coded error reason with the try catch approach I was able to produce an uncaught error and see it in the console. However, when chaining a .catch(() => {}) to the call to restore the error was handled and not raised in the console.

I feel like there is a subtle difference in how these two error handling approaches are working with the promise rejection and it only becomes clear when using an error reason.

@swelham
Copy link
Contributor Author

swelham commented Feb 15, 2022

@BobrImperator is there anything I can do to further assist on this issue?

swelham added a commit to swelham/ember-simple-auth that referenced this issue Feb 23, 2022
It's possible for an `UnhandledPromiseError` to raise in the background when restoring the session. This doesn't raise to the caller of the library but does occur as a caught exception in the browser, which in turn can be picked up by error tracking code such as sentry.

This fixes mainmatter#2358 which has more specific detail around the issue and how to reproduce it.
swelham added a commit to swelham/ember-simple-auth that referenced this issue Mar 14, 2022
It's possible for an `UnhandledPromiseError` to raise when restoring the session. This doesn't raise to the caller of the library but does occur as a caught exception in the browser, which in turn can be picked up by error tracking code such as sentry.

This fixes mainmatter#2358 which has more specific detail around the issue and how to reproduce it.
swelham added a commit to swelham/ember-simple-auth that referenced this issue Mar 15, 2022
It's possible for an `UnhandledPromiseError` to raise when restoring the session. This doesn't raise to the caller of the library but does occur as a caught exception in the browser, which in turn can be picked up by error tracking code such as sentry.

This fixes mainmatter#2358 which has more specific detail around the issue and how to reproduce it.
BobrImperator pushed a commit that referenced this issue Mar 15, 2022
It's possible for an `UnhandledPromiseError` to raise when restoring the session. This doesn't raise to the caller of the library but does occur as a caught exception in the browser, which in turn can be picked up by error tracking code such as sentry.

This fixes #2358 which has more specific detail around the issue and how to reproduce 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 a pull request may close this issue.

2 participants