Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Lib failing to retrying on 5xx if it can't parse the body #154

Closed
SimonWoolf opened this issue Apr 22, 2020 · 1 comment · Fixed by #157
Closed

Lib failing to retrying on 5xx if it can't parse the body #154

SimonWoolf opened this issue Apr 22, 2020 · 1 comment · Fixed by #157
Labels
bug Something isn't working. It's clear that this does need to be fixed.

Comments

@SimonWoolf
Copy link
Member

Customer using ably-go 1.1.2 reporting 400 errors. On investigation this was due to cloudfront returning rendered HTML for 502 errors; ably-go is then unable to parse the body, and turns it into a 400 error.

But this is incorrect - the library should be retrying for any response with an http status code 500-504 (specific spec violation: https://docs.ably.io/client-lib-development-guide/features/#RSC15d), doesn't matter what the body is. It still has the HTTP status code, and can decide to retry based on that.

Possibly introduced by /~https://github.com/ably/ably-go/pull/129/files (though that may have been fixing even-more-broken older behaviour)
@SimonWoolf SimonWoolf added the bug Something isn't working. It's clear that this does need to be fixed. label Apr 22, 2020
@mattheworiordan mattheworiordan mentioned this issue Apr 23, 2020
Closed
gernest added a commit that referenced this issue Apr 23, 2020
@gernest
Properly set Error.StatusCode
8d37982 
fixes #154

newError was completely ingoring  response status code.
It was wrongly deriving it from error code.

This patch ensures Error,StatusCode is set with correct response status.
This was referenced Apr 23, 2020
Merged
Merged
@SimonWoolf
Copy link
Member Author

Thanks for the quick response @gernest. Poke me or quintin when there's a release with this fix in it and we'll let the customer know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working. It's clear that this does need to be fixed.
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Properly set Error.StatusCode ably/ably-go
1 participant
@SimonWoolf