Skip to content

fix: avoid JSON::ParserError for all server errors #582

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

Merged
merged 2 commits into from
Apr 21, 2022
Merged

fix: avoid JSON::ParserError for all server errors #582

merged 2 commits into from
Apr 21, 2022

Conversation

dan-jensen
Copy link
Contributor

@dan-jensen dan-jensen commented Dec 10, 2021

When there is a Twilio service outage that causes the Twilio API to
return a 503 Service Unavailable error, a JSON::ParserError is
raised. This fixes that bug by detecting all 5XX server error status
codes, and handling them the same way 504 status codes were already
being handled.

This is related to the existing bug issue and is basically an expansion of a previous pull request.

Fixes #544

A short description of what this PR does.

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

@dan-jensen
Copy link
Contributor Author

Note: I was tempted to implement the same change for client errors by changing elsif response.status == 400 to use Faraday::Response::RaiseError::ClientErrorStatuses. But that felt like too big a change for this PR.

Also, it makes me nervous that line appears after elsif response.body && !response.body.empty?. I would think we should check for both client errors and server errors before we return the response body, so I'm a bit wary of some special intent there. But if I had to guess, I would say it was an unintentional oversight in the original implementation. I would recommend a follow-up PR to handle client errors.

@dan-jensen
Copy link
Contributor Author

@eshanholtz would you have time to review this? Pinging you because you merged a similar PR earlier this year so I'm guessing you have the most context awareness 🙂

When there is a Twilio service outage that causes the Twilio API to
return a 503 Service Unavailable error, a JSON::ParserError is
raised. This fixes that bug by detecting all 5XX server error status
codes, and handling them the same way 504 status codes were already
being handled.
@doryphores
Copy link

Hello. Any movement on this? Feels like a good fix to merge in.

@childish-sambino childish-sambino changed the title Fix JSON::ParserError for all server errors fix: JSON::ParserError for all server errors Apr 21, 2022
@childish-sambino childish-sambino changed the title fix: JSON::ParserError for all server errors fix: avoid JSON::ParserError for all server errors Apr 21, 2022
Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

LGTM

@childish-sambino childish-sambino merged commit 3175c51 into twilio:main Apr 21, 2022
@dan-jensen dan-jensen deleted the dan-jensen-fix-json-parse-error-for-all-500-responses branch April 21, 2022 16:48
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 this pull request may close these issues.

504 Gateway Time-out caused a JSON::ParserError
4 participants