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

Fix returned EOF error when calling Nodes GC/GcAlloc API #5970

Merged
merged 1 commit into from
Mar 12, 2020

Conversation

jrasell
Copy link
Member

@jrasell jrasell commented Jul 17, 2019

Internally Nomad returns a http.NoBody when calling nodes GC, therefore attempting to decode the body results in an EOF error returned to the user. This fix updates the decodeBody function to check both the response content length and the passed out object.

The following code snippet can be used to locally test:

package main

import (
	"fmt"

	"github.com/hashicorp/nomad/api"
)

func main() {
	nomadClient, _ := api.NewClient(api.DefaultConfig())
	nodeSelf, _ := nomadClient.Agent().Self()
	fmt.Println(nomadClient.Nodes().GC(nodeSelf.Stats["client"]["node_id"], nil))
}

Closes #5506 #5994

api/api.go Outdated Show resolved Hide resolved
@jrasell jrasell changed the title Fix returned EOF error when calling Nodes GC API Fix returned EOF error when calling Nodes GC/GcAlloc API Jul 29, 2019
@tgross
Copy link
Member

tgross commented Nov 6, 2019

@langmartin it looks like the requested changes were made, but I can't get Travis to rebuild it because we've removed the scripts for Travis since this was last pushed. Maybe if @jrasell would be willing to squash his commits and re-push, it'll trigger CircleCI and we can get this merged?

@langmartin
Copy link
Contributor

Yes! @jrasell the age on this is my fault, it got mixed up with some followup work: we shouldn't be using side effecting GET requests for making changes. I'll capture that in another ticket, and this should be fine to merge. @tgross suggestion to squash is right. Sorry about letting this fall through the cracks!

The API decodeBody function will now check the content length
before attempting to decode. If the length is zero, and the out
interface is nil then it is safe to assume the API call is not
returning any data to the user. This allows us to better handle
passing nil to API calls in a single place.
@@ -930,8 +931,16 @@ func parseWriteMeta(resp *http.Response, q *WriteMeta) error {

// decodeBody is used to JSON decode a body
func decodeBody(resp *http.Response, out interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we always skip decoding body if out == nil? I noticed that we wrap few decodeBody invocations with a nil check already:

nomad/api/api.go

Lines 858 to 862 in d890ddb

if out != nil {
if err := decodeBody(resp, &out); err != nil {
return nil, err
}
}
.

It would simplify caller code a bit, and it doesn't seem like an error if caller explicitly passes nil and ignores response body (though body may need to be consumed... a potential bug in current code).

@jrasell jrasell self-assigned this Mar 12, 2020
@jrasell jrasell merged commit 5d5469e into hashicorp:master Mar 12, 2020
@jrasell jrasell deleted the bug-gh-5506 branch March 12, 2020 09:04
jrasell added a commit that referenced this pull request Mar 12, 2020
jrasell added a commit that referenced this pull request Mar 13, 2020
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error while calling go library api [client gc]
4 participants