Skip to content
This repository has been archived by the owner on Jan 26, 2023. It is now read-only.

return error object with status and response #37

Closed
wants to merge 3 commits into from
Closed

return error object with status and response #37

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e29529
return error object with status and response
mcbouslog Mar 10, 2017
ff7e241
edit console log
mcbouslog Mar 13, 2017
dec4bc2
clean-up logs, revert unnecessary changes
mcbouslog Mar 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/make-http-request.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,7 @@ makeHTTPRequest = (method, url, data, headers = {}, modify) ->
if error?.status is 408
resolve makeHTTPRequest.apply null, originalArguments
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the promise resolve if the error status is 408?

Copy link
Contributor

Choose a reason for hiding this comment

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

408 is Request Timeout so retry immediately?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's how I'm reading it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also how I interpreted. Not sure if we want to move this out of here now that we'll be passing along error with status in case of 408s, so relevant handler (in panoptes-client or something in PFE or other app) can decide what it wants to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

My opinion is that the JSON API client shouldn't know anything about how to handle a response. I think its responsibility should be to make requests to a JSON API and pass the response back to a parent app. The parent app can make decisions based on the response.

So, I think retrying on timeouts is perhaps better handled by the Panoptes API client, or by PFE.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that JSON API client should be low level and not make decisions on how to handle responses. My concern with moving it to the Panoptes JS client is if we move this handler to the Panoptes JS client and if we decide to merge the JSON API client and Panoptes JS client, then that separation of concerns wouldn't be present anymore. With that in mind, moving this handler to PFE or moving it to the Panoptes JS client and keeping the clients separate would be best. If moved to PFE, I also don't want us spending a lot of time adding this kind of handling to most requests since I think it might be likely we would want this behavior most of the time. What do you think the best approach here is?

Copy link
Contributor

Choose a reason for hiding this comment

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

My first impression is that the Panoptes client is the best place to make decisions like this. I'm not 100% convinced that the client should repeatedly hammer the API after timeouts, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for the Panoptes client.

I like the behavior that node-request-retry has: https://github.com/FGRibreau/node-request-retry
I added it to the Panoptes subject uploader some time ago and use its default behavior. It'll attempt only up to 5 times with 5 sec intervals depending on the HTTP error.

Copy link
Contributor

Choose a reason for hiding this comment

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

That does sound a lot better. Retrying failed classifications is currently handled in PFE, but I can see how this would be generally useful in the Panoptes client instead.

else if error?
# Prefer rejecting with the response, since it'll have more specific information.
# TODO: Reject with the error as expected and access the response through `error.response` in the handler.
reject response ? error
reject error
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been looking at the error responses returned by Panoptes. They only seem to contain an error message, so we might have to add in error.status and error.statusText here, unless that's already been done elsewhere in the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this highlights another good question. currently the panoptes-javascript-client (p-j-c) just returns an error message, but with this PR and the related p-j-c PR it'll return an error object that includes status and response attributes. the response attribute includes the verbose error message currently returned.

recently I think there was an error related to If-Then headers and the API I didn't quite understand, but the point was made that maybe our error messages should be less verbose or more generic?

if this PR and the related p-j-c PR are merged as is, the error message the PFE will throw to the console will be a less verbose version, but the more verbose explanation will be available if we want to access it.

the example I've been playing with is requesting a project by ID that doesn't exist. here's the error.response attribute:
screen shot 2017-03-14 at 3 25 40 pm
currently the "Could not find project with id-'1693777'" message is returned, but with these changes just "404: Not Found" would be console logged, though the more verbose one would be available as shown above.

If we want to go the route of returning the verbose error message just as we have been, I can clean this p-j-c PR up that I closed?

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment, this version of the client always errors with 'Unknown error (bad response) in the Panoptes client, because error is not an instance of Error ie. not a JavaScript error.

I think it got lost in the noise on Slack, but I've set up a copy of PFE using these branches of the client libs on https://api-client-errors.pfe-preview.zooniverse.org/

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out there's a build step I'd missed when deploying the coffeescript changes. Should be sorted out now.

else
resolve response

Expand Down