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

Implement http trailers for error handling #1519

Merged
merged 10 commits into from
Jul 28, 2015
Merged

Implement http trailers for error handling #1519

merged 10 commits into from
Jul 28, 2015

Conversation

whyrusleeping
Copy link
Member

This PR started off as a quick fix for the get command, but turned into a moderately large refactor of the http commands library.

jbenet and others added 4 commits July 24, 2015 14:41

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
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>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
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>
@jbenet jbenet added the status/in-progress In progress label Jul 25, 2015
@whyrusleeping
Copy link
Member Author

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>
@whyrusleeping
Copy link
Member Author

I'm running into the random API failures issue again on this branch...

@jbenet
Copy link
Member

jbenet commented Jul 27, 2015

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" {
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

addressed

@jbenet
Copy link
Member

jbenet commented Jul 27, 2015

some comments above, but overall looking good. almost there?

only worried about:

  • the silent error in readStreamedJson
  • flushCopy being used for or fixing something
  • "I'm running into the random API failures issue again on this branch..."

Sorry, something went wrong.

@whyrusleeping
Copy link
Member Author

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>
@whyrusleeping
Copy link
Member Author

@jbenet github decided to hide this: #1519 (comment)

@jbenet
Copy link
Member

jbenet commented Jul 27, 2015

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.

was this the thing that creating a transport fixed?

@whyrusleeping
Copy link
Member Author

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>
@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

@whyrusleeping this LGTM. let's do it!

jbenet added a commit that referenced this pull request Jul 28, 2015
Implement http trailers for error handling
@jbenet jbenet merged commit aaaf884 into master Jul 28, 2015
@jbenet jbenet removed the status/in-progress In progress label Jul 28, 2015
@jbenet jbenet deleted the get-fix branch July 28, 2015 00:56
@jbenet jbenet modified the milestone: IPFS 0.3.6 Jul 28, 2015
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.

None yet

2 participants