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

S3 HeadBucket returns EOF when attempting to deserialize error response. #798

Closed
skmcgrail opened this issue Oct 6, 2020 · 2 comments
Closed
Labels
bug This issue is a bug.

Comments

@skmcgrail
Copy link
Member

skmcgrail commented Oct 6, 2020

The Go HTTP Client does not allow for body responses to be populated for HEAD HTTP methods. This means that regardless of whether the server responds with a body the Go client will assign a net/http.noBody type as the response body. This will result in a EOF error from the deserializer which assumes that an XML document can be parsed. We should modify our error deserialization behavior to account for HEAD responses not having a return body present, and should explore having an error that at least surfaces the HTTP StatusCode and Message to the user.

@skmcgrail skmcgrail added the bug This issue is a bug. label Oct 6, 2020
@jasdel
Copy link
Contributor

jasdel commented Oct 6, 2020

Not just HEADs in general the SDK probably should not error at all (error or success response) if there is no response payload. The only exception to this being the S3 200 error API customizations. In all other cases the SDK should assume empty response body (e.g. EOF) is OK and should be allowed without failure.

There are some APIs that model no response, but actual return something back which does fit the protocol's format. I think one of the IoT APIs returns a OK literal string (unquoted) that will fail decode because its not valid JSON. But in this case the SDK should not of attempted to deserialize the response at all, and instead just thrown it away. Either delegate to a resp.Body.Close and optional with io.Copy(ioutil.Discard, resp.Body) as well. (discard isn't really needed, Close will do this automatically. (may be needed for eventstream though, but we can investigate that then)

@jasdel
Copy link
Contributor

jasdel commented Oct 16, 2020

This issue has been fixed in #801, and will be included in the SDK's next release.

@jasdel jasdel closed this as completed Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

No branches or pull requests

2 participants