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.

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

thrift-client: improve non-200 response error handling #9

Merged
merged 5 commits into from
Jun 9, 2022

Conversation

RobWiggins
Copy link

@RobWiggins RobWiggins commented Jun 6, 2022

When a non-200 response is received, reject with a new ThriftRequestError, which is an instance of error.

Rejecting with a payload that is an instance of an Error is the proper thing to do. Doma's logger handles this better.

DCB-689

@RobWiggins RobWiggins marked this pull request as ready for review June 6, 2022 13:38
@RobWiggins RobWiggins requested review from strmer15, markdoliner-doma and a team June 6, 2022 13:38
Copy link

@strmer15 strmer15 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

@mabranon mabranon left a comment

Choose a reason for hiding this comment

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

LGTM

@RobWiggins RobWiggins merged commit 9d3fc2b into main Jun 9, 2022
@RobWiggins RobWiggins deleted the rwiggins/improve-non-200-thrift-errors-DCB-689 branch June 9, 2022 16:16
markdoliner-doma added a commit that referenced this pull request Jul 5, 2022
We [recently added some better exception handling](#9) but unfortunately printing the response body didn't work because [`response.text()` is a Promise](https://github.github.io/fetch/#response-body). The message looks like this: `ThriftRequestError: Thrift request failed: HTTP 503 Service Unavailable: [object Promise]`

The fix is to use `.then()` to get the body.
markdoliner-doma added a commit that referenced this pull request Jul 6, 2022
We [recently added some better exception handling](#9) but unfortunately printing the response body isn't working because [`response.text()` is a Promise](https://github.github.io/fetch/#response-body). The message looks like this: `ThriftRequestError: Thrift request failed: HTTP 503 Service Unavailable: [object Promise]`

The fix is to use `.then()` to get the body.

I have not tested this. I did run `npm run test` and some of the tests pass, but 75 out of 108 thrift-integration tests fail. I'm seeing this error a lot:
```
request to http://localhost:8090/thrift failed, reason: connect ECONNREFUSED ::1:8090
```
I think it also might have been happening before this change. It looks unrelated. It seems like it's happening because [Hapi listens on 127.0.0.1](https://hapi.dev/api/?v=20.2.2#-serveroptionsaddress) and [Node v17 and higher stopped forcing IPv64addresses to be listed first when resolving domains](nodejs/node#40537 (comment)) so now IPv6 addresses can be returned first. I'll probably post a separate PR for that, but for now I'd prefer not to block this change.
markdoliner-doma added a commit that referenced this pull request Jul 6, 2022
In #9 we changed thrift-client to return an instance of ThriftRequestError when the client gets an HTTP error. This commit exports the ThriftRequetsError class and fixes some test assertions in light of those changes.

I feel good about it!
markdoliner-doma added a commit that referenced this pull request Jul 6, 2022
In #9 we changed thrift-client to return an instance of ThriftRequestError when the client gets an HTTP error. This commit exports the ThriftRequestError class and fixes some test assertions in light of those changes.

I feel good about it!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants