fix Promise error handling #3022

alicewriteswrongs · 2017-04-07T15:32:59Z

What are the relevant tickets?

none

What's this PR do?

I was looking at a JavaScript error in Sentry this morning and I realized (after reading this issue, among others) that our error handling for Promises is done incorrectly, in a way that can swallow errors and lead to some strange behavior.

In short, these two things are not equivalent:

// pattern we have in our code a fair amount right now:
promise.then(success => {
  doSomethingWithSuccess(success);
  return Promise.resolve(success); // maybe, we don't always do this
}).catch(error => {
  ohNoAnError(error); // :(
  return Promise.reject();
};

// correct pattern
promise.then(success => {
  doSomethingWithSuccess(success);
  return Promise.resolve(success); // maybe, we don't always do this
}, error => {
  ohNoAnError(error); // :(
  return Promise.reject();
});

basically, the first problem is an issue because .catch becomes overly broad. Instead of just handling the rejected case for promise, it also catches any errors that happen in the .then handler. This can, in certain cases, include rendering / UI errors which are not related to the promise itself, but related to an error in the code we use to handle or render the data once it comes back. In this case, the UI error will be silently swallowed by the .catch (because it's occurring in the .then handler) and we won't see it. Then, often, the error will show up somewhat cryptically with a different stack trace.

this is a problem in particular with redux, because our async (redux-thunk) action dispatchers trigger a render() call when dispatch fires, so that if there is an error anywhere in that render() call (say, because of wacky data) it will trace back to the dispatch() and be silently swallowed by .catch. yucky.

The correct way to use promises is .then(onResolve, onReject). Then our handler functions are strictly confined to dealing with resolution or rejection of the original promise itself. Of course, if want to chain off of this function that is fine and dandy as well, but we need to make sure that there is an onReject handler for the initial promise beforehand, otherwise the error can be propagated somewhat strangely.

Anyway, I believe this is the root cause of a React error we're seeing rarely but consistently (Cannot read property 'getHostNode' of null).

two other React issues relating to this:

facebook/react#8267

facebook/react#4199

How should this be manually tested?

tests should pass, app should behave normally, there should be no regressions.

codecov-io · 2017-04-07T15:33:02Z

Codecov Report

Merging #3022 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3022      +/-   ##
==========================================
+ Coverage   98.25%   98.25%   +<.01%     
==========================================
  Files         253      253              
  Lines       12029    12050      +21     
==========================================
+ Hits        11819    11840      +21     
  Misses        210      210

Impacted Files	Coverage Δ
static/js/actions/index.js	`100% <ø> (ø)`	⬆️
static/js/containers/App_test.js	`100% <ø> (ø)`	⬆️
static/js/lib/redux_rest.js	`100% <ø> (ø)`	⬆️
static/js/containers/DashboardPage.js	`86.59% <ø> (ø)`	⬆️
static/js/actions/email.js	`100% <ø> (ø)`	⬆️
static/js/actions/course_enrollments.js	`100% <ø> (ø)`	⬆️
static/js/lib/api.js	`100% <100%> (ø)`	⬆️
static/js/actions/course_prices.js	`100% <100%> (ø)`	⬆️
static/js/lib/api_test.js	`99.66% <100%> (ø)`	⬆️
static/js/actions/programs.js	`100% <100%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7dce63f...f4d5dbe. Read the comment docs.

noisecapella · 2017-04-07T15:45:29Z

If you have a promise chain like this:

// correct pattern
fetchSomething().then(() => transformIt()).then(success => {
  doSomethingWithSuccess(success);
  return Promise.resolve(success); // maybe, we don't always do this
}, error => {
  ohNoAnError(error); // :(
  return Promise.reject();
});

We would be handling errors from either transformIt or fetchSomething, right? And if we added the catch instead of using onReject then we would also handle errors happening from doSomethingWithSuccess? Just making sure

alicewriteswrongs · 2017-04-07T15:53:35Z

Yes, exactly. And, in particular, in our action dispatchers, which look like this:

export function fetchProgramEnrollments(): Dispatcher<AvailablePrograms> {
  return (dispatch: Dispatch) => {
    dispatch(requestGetProgramEnrollments());
    return api.getPrograms().
      then(enrollments => dispatch(receiveGetProgramEnrollmentsSuccess(enrollments))).
      catch(error => {
        dispatch(receiveGetProgramEnrollmentsFailure(error));
        // the exception is assumed handled and will not be propagated
      });
  };
}

the .catch handler will swallow any errors which happen as a result of the dispatch call. This might seem ok but it is not - dispatch is from react-redux, and a call to dispatch will trigger a re-render of the UI. Any UI errors that happen as a result of that re-render will therefore be in the same call stack, and so will happen in the .then handler, and will then be silently swallowed by the .catch handler. Not what we want!

This comment probably explains the problem better than I will be able to.

noisecapella · 2017-04-07T16:13:32Z

In api.js:125 there's a catch we should probably move into the preceeding then

noisecapella · 2017-04-07T16:15:57Z

static/js/actions/programs.js

@@ -50,7 +48,7 @@ export const receiveAddProgramEnrollmentSuccess = createAction(RECEIVE_ADD_PROGR
 export const RECEIVE_ADD_PROGRAM_ENROLLMENT_FAILURE = 'RECEIVE_ADD_PROGRAM_ENROLLMENT_FAILURE';
 export const receiveAddProgramEnrollmentFailure = createAction(RECEIVE_ADD_PROGRAM_ENROLLMENT_FAILURE);

-export const addProgramEnrollment = (programId: number): Dispatcher<AvailableProgram> => {
+export const addProgramEnrollment = (programId: number): Dispatcher<?AvailableProgram> => {


Why do we need to mark this with ??

because the onResolved handler doesn't actually return a Promise<AvailableProgram>, it just returns undefined.

before I think Flow wasn't able to accurately type-check things.

alicewriteswrongs · 2017-04-07T20:06:00Z

ok! I re-wrote fetchJSONWithCSRF to have some cleaner handling of the different cases we need to deal with, and to just have one .then call off of response.text().

noisecapella · 2017-04-07T21:20:32Z

static/js/lib/api.js

@@ -92,12 +93,17 @@ import { fetchWithCSRF } from './api';
 *  - non 2xx status codes will reject the promise returned


Can you move this comment down to _fetchJSONWithCSRF?

And also add a comment explaining about the conversion from maybe to promise in case someone isn't familiar with sanctuary

pdpinch · 2017-04-09T20:15:55Z

https://sentry.io/mit-office-of-digital-learning/micromasters/issues/249296145/

pdpinch · 2017-04-09T20:16:17Z

https://sentry.io/mit-office-of-digital-learning/micromasters/issues/228916007/

noisecapella

Just a comment about a comment, looks good 👍

noisecapella · 2017-04-10T14:32:45Z

static/js/lib/api.js

@@ -84,6 +85,15 @@ const _fetchWithCSRF = (path: string, init: Object = {}): Promise<*> => {
 export { _fetchWithCSRF as fetchWithCSRF };
 import { fetchWithCSRF } from './api';

+const resolveEither = S.either(


Can you add a comment here in case someone is unfamiliar with sanctuary?

In many places where we use Promises (e.g. for making fetch requests to our backend APIs) we were previously incorrectly handling promise rejection. We were often doing `promise.then(fn).catch(fn)`, which is not a great idea - then the `.catch` handler catches any errors in the Promise AND any errors in the `.then` handler. Instead, we should prefer to do `promise.then(resolve, reject)` - this makes the `reject` handler narrower in it's responsibility. The main problem with the former pattern is that the `.catch` handler can swallow any exceptions which occur in the `.then` handler, and we will not see them. This can lead to strange and unhelpful stack traces (such as the react `getHostNode` error that crops up sometimes). Anyway, this PR should fix it! I also simplified and streamlined our `fetchJSONWithCSRF` function while I was at it. pr #3022

odlbot temporarily deployed to micromasters-ci-pr-3022 April 7, 2017 15:33 Inactive

alicewriteswrongs added the Needs Review label Apr 7, 2017

noisecapella self-assigned this Apr 7, 2017

noisecapella reviewed Apr 7, 2017

View reviewed changes

noisecapella added Waiting on Author and removed Needs Review labels Apr 7, 2017

alicewriteswrongs force-pushed the ap/promises branch from d19f2b2 to 17558e9 Compare April 7, 2017 20:04

alicewriteswrongs temporarily deployed to micromasters-ci-pr-3022 April 7, 2017 20:04 Inactive

alicewriteswrongs force-pushed the ap/promises branch from 17558e9 to 49c517e Compare April 7, 2017 20:05

alicewriteswrongs temporarily deployed to micromasters-ci-pr-3022 April 7, 2017 20:05 Inactive

alicewriteswrongs force-pushed the ap/promises branch from 49c517e to c97d942 Compare April 7, 2017 20:13

alicewriteswrongs temporarily deployed to micromasters-ci-pr-3022 April 7, 2017 20:13 Inactive

alicewriteswrongs added Needs Review and removed Waiting on Author labels Apr 7, 2017

alicewriteswrongs force-pushed the ap/promises branch from c97d942 to d4b911e Compare April 7, 2017 21:03

alicewriteswrongs temporarily deployed to micromasters-ci-pr-3022 April 7, 2017 21:04 Inactive

noisecapella reviewed Apr 7, 2017

View reviewed changes

noisecapella added Waiting on Author and removed Needs Review labels Apr 10, 2017

alicewriteswrongs force-pushed the ap/promises branch from d4b911e to f4d5dbe Compare April 10, 2017 14:12

alicewriteswrongs deployed to micromasters-ci-pr-3022 April 10, 2017 14:12 View deployment

noisecapella approved these changes Apr 10, 2017

View reviewed changes

alicewriteswrongs force-pushed the ap/promises branch from f4d5dbe to 824fd75 Compare April 10, 2017 14:44

alicewriteswrongs merged commit 824fd75 into master Apr 10, 2017

alicewriteswrongs deleted the ap/promises branch April 10, 2017 14:44

alicewriteswrongs temporarily deployed to micromasters-ci April 10, 2017 14:55 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix Promise error handling #3022

fix Promise error handling #3022

Uh oh!

alicewriteswrongs commented Apr 7, 2017 •

edited

Loading

Uh oh!

codecov-io commented Apr 7, 2017 •

edited

Loading

Uh oh!

noisecapella commented Apr 7, 2017

Uh oh!

alicewriteswrongs commented Apr 7, 2017

Uh oh!

noisecapella commented Apr 7, 2017

Uh oh!

noisecapella Apr 7, 2017

Uh oh!

alicewriteswrongs Apr 7, 2017

Uh oh!

alicewriteswrongs Apr 7, 2017

Uh oh!

alicewriteswrongs commented Apr 7, 2017

Uh oh!

noisecapella Apr 7, 2017

Uh oh!

noisecapella Apr 10, 2017

Uh oh!

pdpinch commented Apr 9, 2017

Uh oh!

pdpinch commented Apr 9, 2017

Uh oh!

noisecapella left a comment

Uh oh!

noisecapella Apr 10, 2017

Uh oh!

Uh oh!

		@@ -92,12 +93,17 @@ import { fetchWithCSRF } from './api';
		* - non 2xx status codes will reject the promise returned

fix Promise error handling #3022

fix Promise error handling #3022

Uh oh!

Conversation

alicewriteswrongs commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Uh oh!

codecov-io commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

noisecapella commented Apr 7, 2017

Uh oh!

alicewriteswrongs commented Apr 7, 2017

Uh oh!

noisecapella commented Apr 7, 2017

Uh oh!

noisecapella Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

alicewriteswrongs Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

alicewriteswrongs Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

alicewriteswrongs commented Apr 7, 2017

Uh oh!

noisecapella Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

noisecapella Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

pdpinch commented Apr 9, 2017

Uh oh!

pdpinch commented Apr 9, 2017

Uh oh!

noisecapella left a comment

Choose a reason for hiding this comment

Uh oh!

noisecapella Apr 10, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alicewriteswrongs commented Apr 7, 2017 •

edited

Loading

codecov-io commented Apr 7, 2017 •

edited

Loading