Prevent UnhandledPromiseError when restoring the session

[swelham]

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 #2358 which has more specific detail around the issue and how to reproduce it.

[BobrImperator]

Hi, thanks for working on this!
Let me just quickly say that I didn't ignore your issue, I simply forgot to reply 🙈

I don't exactly think this is valid, it feels like we're just silencing an actual problem in the code.
It seems like sentry is hooking up to RSVP.on('error') directly for some reason, which I'm not exactly sure is correct, but if some error is thrown to there we should fix it instead.

It'd be nice to reproduce it and cover with a test, I could try taking a look at it.

[swelham]

Hey, no problem at all.

I tried writing a test for this but I was unable to reproduce it. I think it's because the try/catch will swallow any error thrown by the promise. Though I didn't think to try using RSVP.on('error'), maybe that would work? If we used the .catch() suggested in this PR would we still need the try/catch? Without that I believe it would be more testable.

The error that is being thrown comes from the use of this const https://github.com/simplabs/ember-simple-auth/blob/master/packages/ember-simple-auth/addon/internal-session.js#L69 when it's passed as the callback to the promise, which I think is a valid use of that. However, because the promise isn't directly handled by the try/catch it's still regeristing an exception.

[BobrImperator]

Here's a reproduction of what might be happening here https://ember-twiddle.com/939eb4a8aec7f6b188cfc281140191e6?openFiles=controllers.application%5C.js%2C.
Interestingly Native promise doesn't throw any errors when in try catch block, but RSVP promise does throw.

I think we could move forward with this PR but it'd be better to drop the try catch block I think.

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

[swelham]

Thanks for the detailed response and twiddle.

I have updated the PR with the suggested code changes.

[marcoow]

I don't quite understand this tbh – there shouldn't be a difference between try/catch and .catch(… – if there is it seems that would be because your error tracking might hook into the wrong event or so?

Admittedly it wasn't a great API choice to have the session's restore method raise an error if there's no restorable state in the session data but I wouldn't want to go back to raw promise APIs unless we absolutely have to…

[BobrImperator]

@marcoow RSVP promises seem to still emit errors to RSVP.on('error') which is a hook that sentry uses (for whatever reason). Also ultimately this is a regression on ESA's part as well, since the old code was actually using .catch method while currently it uses try catch instead.

ESA used to pass a function that ran beforeModel hook inside a promise rejection handler.
https://github.com/simplabs/ember-simple-auth/blob/master/packages/ember-simple-auth/addon/initializers/setup-session-restoration.js#L38

[marcoow]

Also ultimately this is a regression on ESA's part as well, since the old code was actually using .catch method while currently it uses try catch instead.

Hm, yeah – I think that's a reasonable argument for changing this 🤔

[BobrImperator]

It seems like one test is failing now because the stubbed restore has a wrong return value 😞

It seems it should be sinon.stub(session, 'restore').returns(RSVP.reject())

[swelham]

Ah, sorry I should have caught that before pushing that change.

Test should now be passing on CI.

[BobrImperator]

Thank you!

[BobrImperator]

@swelham released https://github.com/simplabs/ember-simple-auth/blob/master/CHANGELOG.md

[swelham]

Thanks @BobrImperator 👍