-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Implement http trailers for error handling #1519
Conversation
License: MIT Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
Get had a random timeout of 60s. This commit fixes that, wiring up our contexts correctly. License: MIT Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
stream output might break. in these cases we need to notify the client. this is after a 200 response has been sent. We do this by setting a special trailer (header after the body): X-Stream-Error: <error cause> This is similar to what's done by systems like gRPC. This still needs to be read + handled on the other side. License: MIT Signed-off-by: Juan Batiz-Benet <juan@benet.ai>
refactor http handler and copyChunks to get this all to work correctly License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
Theres still some more refactoring i want to do, so its not quite ready yet. (I just wanted travis and circle to test it for me) |
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
I'm running into the random API failures issue again on this branch... |
how did we fix those? |
if len(httpRes.Header.Get(streamHeader)) > 0 { | ||
// if output is a stream, we can just use the body reader | ||
res.SetOutput(httpRes.Body) | ||
if contentType != "application/json" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are constants elsewhere for things like these... a simple typo could cause many headaches. let's have the type system help us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
some comments above, but overall looking good. almost there? only worried about:
|
the random failures go away if i make my own http client and disable keepalives. I think that even with the closing of the response bodies, theres still some sort of race condition in the http lib that needs to be fixed before we can rely on it. |
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
@jbenet github decided to hide this: #1519 (comment) |
was this the thing that creating a transport fixed? |
yes. |
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
@whyrusleeping this LGTM. let's do it! |
Implement http trailers for error handling
This PR started off as a quick fix for the get command, but turned into a moderately large refactor of the http commands library.