To prevent system crashes, Node should parse exceptions
paulalbert1 opened this issue · 9 comments
Background
- With issue #53, we noted how the app seems to frequently crash. This occurs in both dev and prod.
- We speculated that this was a problem with session management, but @ganga ruled that out.
- Since then, @sarbajitdutta pinpointed a more likely cause: improper exception handling by the Publication Manager app itself.
Reproducing the error
- As an example, consider user = "las2012"
- If we run Feature Generator or Identity APIs on Swagger (https://reciter.weill.cornell.edu/reciter/swagger-ui.html), the system throws a 404 exception. This does not affect uptime.
- Now if we make a request via the dev or prod app like so http://[reciter-publication-manager-URL]]/app/las2012, we see the following error.
https://reciter.weill.cornell.edu/reciter/feature-generator/by/uid?uid=las2012&totalStandardizedArticleScore=4&useGoldStandard=AS_EVIDENCE&analysisRefreshFlag=0&retrievalRefreshFlag=FALSE&filterByFeedback=ALL
POST /users/reciter/validate 200 0.598 ms - 31
undefined:1
The uid provided 'las2012' was not found in the Identity table
^
SyntaxError: Unexpected token T in JSON at position 0
at JSON.parse (<anonymous>)
at Request._callback (/usr/src/app/src/api/reciterPublication.js:30:25)
at Request.self.callback (/usr/src/app/node_modules/request/request.js:185:22)
at Request.emit (events.js:311:20)
at Request.<anonymous> (/usr/src/app/node_modules/request/request.js:1154:10)
at Request.emit (events.js:311:20)
at IncomingMessage.<anonymous> (/usr/src/app/node_modules/request/request.js:1076:12)
at Object.onceWrapper (events.js:417:28)
at IncomingMessage.emit (events.js:323:22)
at endReadableNT (_stream_readable.js:1204:12)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! reciter-publication-manager@0.0.0 start: `NODE_ENV=production node ./bin/www`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the reciter-publication-manager@0.0.0 start script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
- As you can see the system is returning a 404 error as opposed to JSON, which crashes npm (package manager). Because the system is not anticipating error exceptions, the entire system crashes via a domino effect.
Suggested change
- In cases of exceptions / errors, Publication Manager should not try to parse JSON.
- Instead the system should only attempt to parse JSON output in cases where a 200 code is returned.
- In all other cases, consistent with best practices, Node should parse any errors and then share the error with the user. This should be done in a way that does not crash npm.
@paulalbert1 @ganga The error handling should take place across all the nodejs APIs i.e. all the api configured to use reciter, reciter-pubmed and recietr-scopus apis. This would make the system more resilient and have proper error handling at all levels.
@sarbajitdutta , got it. I think we should create a component to show the error message and an option to go back if applicable so that any one can use it.
This component will be used whenever the backend gives any resonse other than 200.
Let me know your thoughts.
@ganga Ideally you want proper responses to the frontend based on the responses you get from the upstream apis. E.g. if its a user not found return code(404) from feature generator API. Then the nodeJS Api would not try to look for a json parse and return appropriate response to the frontend. Your idea of having a blanket component would also work for error arriving due to issues other than 404. Such as http(500) or (502/503) then an appropriate error handler component page should be shown to user.
@sarbajitdutta , One thing is that validate API when returning 503 or 502 is giving html output instead of json.
Because of this we have issues in WithAuth.jsx.
fetch('/users/reciter/validate', { method: "POST" }) .then(r => r.json())
r.json is throwing error as the response is not json
Also, as of now, when any error occurs like the one above or if the response is not 200, then app is redirecting to Login page as the state is being set to redirect: true.
.catch(err => { return this.setState({ loading: false, redirect: true }); });
Redirect to login page also happens in Auth.jsx
const { loading, redirect } = this.state; if (redirect) { return <Redirect to="/login" />; }
So issues here:
- backend is giving 502/503 response regularly atleast on dev env.
- Validate API is returning HTML output below
`
503 Service Temporarily Unavailable
`Doubt:
if validate API is receving 502/503, should we redirect to login page?
Note: I have made changes to show error component if API returns any error. But with Validate API having issues, the userexperience is not so smooth.
My changes to show error component only works if validate API gives 200 and other API throws error.
If validate API also returns 500, it is redirecting to Login page as per previous component.
@ganga can you tell me which backend services(api) are giving 502/503 errors. The backend services have a load balancer in front of them which gives a basic response when the backend services is not reachable. If you can elaborate which services I can return appropriate json response when load balancer has 502 /503 errors. Can you let me know what kind of JSON response you are expecting? I can model that in when LB hits that error. That way you can configure properly to parse the json even on 502/503 and take appropriate actions.
issue is only with Validate API. which is giving HTML response on 502/503. If this is fixed, it would be great.
But the issue here is, when Validate API give 502/503 response, then page is redirecting to login page though session is valid.
@ganga - For http 500 errors the actual error should be shown and then action should be shown how to fix it. Like in this case it's more than 2000 results. The error message should be parsed and then shown appropriately
Here's how this is occurring in the current version of dev.
pubmed_change_dataERROR.mov.zip
More graceful error handling has been added to Dev