Fixes #1324: Properly closes connections in tribe #1327

IRCody · 2016-11-01T22:43:23Z

Fixes #1324

Summary of changes:

adds proper http.Response.Close() calls. Primarily in worker.go (which causes this issue) but also around some of the client calls that I think could cause similar issues.

Testing done:

Reproduced the bug in Open connections from snap are not closed #1324 reliably, implemented the fix, and tested via same method and verified that the number of open connections isn't constantly growing.

@intelsdi-x/snap-maintainers

thomastaylor312 · 2016-11-02T16:55:21Z

mgmt/tribe/worker/worker.go

 		if resp.StatusCode == 200 {
 			if resp.Header.Get("Content-Type") != "application/x-gzip" {
 				logger.WithField("content-type", resp.Header.Get("Content-Type")).Error("Expected application/x-gzip")
 			}
 			dir, err := ioutil.TempDir("", "")
 			if err != nil {
 				logger.Error(err)
+				resp.Body.Close()


Should this just be a defer right after the c.TribeRequest() call? It would get rid of the Body.Close() calls below

That's a good suggestion, thanks!

edit:
Actually if you look at context, this is running in a for loop and we need to close this after every run, not just at the end of the function call.

Yeah, makes sense.

too many resp.Body.Close() in every error. Should it be added in the source?

So maybe move this part to separate function (and make this one less blobby), or just wrap it by ad hoc function

func() { ... resp := Do() iferr... defer resp.Body.Close() ... }()

candysmurf · 2016-11-03T17:46:11Z

I just want to explore a different option. Don't you think by doing
defer rsp.Close()

in (c _Client) TribeRequest() (_http.Response, error) method is better than doing resp.Close() in every error of the loop?

candysmurf · 2016-11-03T17:57:29Z

mgmt/tribe/worker/worker.go

 		if resp.StatusCode == 200 {
 			if resp.Header.Get("Content-Type") != "application/x-gzip" {
 				logger.WithField("content-type", resp.Header.Get("Content-Type")).Error("Expected application/x-gzip")
 			}
 			dir, err := ioutil.TempDir("", "")
 			if err != nil {
 				logger.Error(err)
+				resp.Body.Close()


too many resp.Body.Close() in every error. Should it be added in the source?

lmroz · 2016-11-03T18:02:04Z

Go Documentation says

The default HTTP client's Transport does not
attempt to reuse HTTP/1.0 or HTTP/1.1 TCP connections
("keep-alive") unless the Body is read to completion and is
closed.

So it's good idea to read contents even if not used before closing.

lmroz · 2016-11-03T18:09:55Z

Cannot add line comment to unchanged line, but here:

 func httpRespToAPIResp(rsp *http.Response) (*rbody.APIResponse, error) {
    if rsp.StatusCode == 401 {
        return nil, fmt.Errorf("Invalid credentials")  <<<<<<<<<<<<<<<<<
    }
    resp := new(rbody.APIResponse)
    b, err := ioutil.ReadAll(rsp.Body)
    rsp.Body.Close()

response body still has to be closed.

IRCody · 2016-11-03T19:06:32Z

@lmroz, @candysmurf: I'm sure the mutliple rsp.Body.Close() calls can in all cases be reduced. When I was doing this fix I was focused on making it work. I can work on a version that is "cleaner" and refactors some of the ways we do things around http.Requests.

@lmroz: The code your linking already has the response body closed (I believe) because of the defer rsp.Body.Close() calls that happen prior to going into that function.

lmroz · 2016-11-03T20:50:11Z

@IRCody Right, looks like body is closed just after this function is called. However this function is calling .Close() anyways, and that led me to believe that closing is responsibility of this function. IMO it's good to decide which code is responsible for this. In case of calling .Close() twice current implementation just ignores it and doesn't even return error, however seems like it's not defined behavior.

IRCody · 2016-11-03T21:16:00Z

@lmroz: I agree there is room for improvement. This code started out as debugging a live issue and iterating on fixing the issue. I need to go back and refactor so it's a cleaner fix. You're right that multiple calls to Close() are implementation defined.

IRCody · 2016-11-07T22:54:29Z

@lmroz, @candysmurf, @thomastaylor312:

I added two commits that I think Improve this PR. Before merging I'd like to merge the first two but I wanted to leave it as a separate commit for the moment so it's easy to see what changes have been made from the initial review.

The first commit addresses the comments in this PR around duplicate calls to Body.Close(). I refactored the downloading plugin part into another function that handles this so there is just one defer call to body.Close().

The second commit fixes an issue I noticed in the client where I think we are leaking http.Transports on client creation. The fix makes use of something that was added in go 1.7 (IdleConnTimeout in http.Transport).

Since this was added in 1.7 the 1.6.3 tests don't work. I talked with @nanliu about it we were wondering if it makes sense to peg Snap to more than the latest version of Go. Since the released binaries will always be the latest version. I was hoping to get some opinions on that here since it's relevant.

If we decide to continue doing latest-1 support I can update this PR to not have IdleConnTimeout and resubmit that change once go 1.8 is released.

/cc @intelsdi-x/snap-maintainers

nanliu · 2016-11-07T23:05:43Z

We only build binaries with go 1.7.1. Since this is an application and not a library, we can safely drop 1.6.3. This will also reduce the test matrix significantly.

geauxvirtual · 2016-11-07T23:28:25Z

We only build binaries with go 1.7.1. Since this is an application and not a library, we can safely drop 1.6.3. This will also reduce the test matrix significantly.

This makes the assumption that everyone using snap is using a release binary and is not building off of master.

IRCody · 2016-11-08T00:19:36Z

We only build binaries with go 1.7.1. Since this is an application and not a library, we can safely drop 1.6.3. This will also reduce the test matrix significantly.

This makes the assumption that everyone using snap is using a release binary and is not building off of master.

Or that they are comfortable running the latest version of Go. We already require relatively newer releases of go by only testing/supporting the 2 most recent releases.

lmroz · 2016-11-08T12:39:36Z

mgmt/tribe/worker/worker.go

+		"_block":         "download-plugin",
+	})
+	resp, err := c.TribeRequest()
+	defer resp.Body.Close()


Nil pointer dereference possible here (on error TribeRequest will return nil response).

Good catch. I moved the defer below the err handling for tribeRequest.

lmroz · 2016-11-08T12:45:52Z

mgmt/tribe/worker/worker.go

+			logger.Error(err)
+			return nil, err
+		}
+		f, err := os.Create(path.Join(dir, fmt.Sprintf("%s-%s-%d", plugin.TypeName(), plugin.Name(), plugin.Version())))


Not related to PR but Create is just return OpenFile(name, O_RDWR|O_CREATE|O_TRUNC, 0666), so chmod can be done in more compact way.

Modified this so it's just one call to OpenFile.

lmroz · 2016-11-08T12:47:07Z

mgmt/tribe/worker/worker.go

+		if resp.Header.Get("Content-Type") != "application/x-gzip" {
+			logger.WithField("content-type", resp.Header.Get("Content-Type")).Error("Expected application/x-gzip")
+		}
+		dir, err := ioutil.TempDir("", "")


Not related to PR but maybe this code should delete this dir (why dir?) and file in case of error.

It won't be created if there is an error?

But error during Create/Chmod (these are rather unlike) will leave that directory and function just eats the path.

thomastaylor312 · 2016-11-10T19:02:08Z

I agree with @nanliu and @IRCody on this one. Also, I would love to have this fix because it does cause issues with running tribe in Kubernetes that have the possibility of getting worse with scale

andrzej-k · 2016-11-15T11:32:46Z

mgmt/rest/client/client.go

@@ -101,6 +102,17 @@ func Username(u string) metaOp {
 	}
 }

+var (
+	secureTransport = &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},


InsecureSkipVerify: true shouldn't this be set to false for secure transport?

Good catch @andrzej-k. I updated with the correct settings.

andrzej-k · 2016-11-15T11:33:48Z

mgmt/rest/client/client.go

+		IdleConnTimeout: time.Second,
+	}
+	insecureTransport = &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: false},


false -> true ?

IRCody · 2016-11-15T18:00:11Z

I factored out the IdleConnTimeout for the transports so that this fix can be merged. I'll submit a separate PR with the ConnTimeout (that requires go1.7+) so discussion on that can happen separately.

cc: @candysmurf

candysmurf · 2016-11-15T21:15:28Z

mgmt/tribe/worker/worker.go

+			return nil, err
+		}
+		io.Copy(f, resp.Body)
+		f.Close()


if using defer f.Close(), it'll take care of the error case and force close it anyway. will it?

candysmurf · 2016-11-15T21:33:28Z

mgmt/tribe/worker/worker.go

+			logger.Error(err)
+			return err
+		}
+		if w.isPluginLoaded(plugin.Name(), plugin.TypeName(), plugin.Version()) {


add a comment. Making sure the plugin is loaded. Otherwise, I thought it should check this in the beginning before loading a plugin. Then I saw there is a similar check at the beginning. If no error is not good enough when loading a plugin?

I don't understand what you're saying.

I meant to say:

method: w.pluginManager.Load(rp) should give enough info of if a plugin is loaded or not. Why we need to check it again in line 343. If line 343 is needed, please add a comment to it.

I understand now. I'm not sure I can give the original reasoning behind this as this section was just refactored in this PR, not modified. I think it's better to leave it as-is.

Properly closes some http.Respons's that were not previously closed.

Avoids leaking http.Transports on client creation by re-using transports. Removes duplicate close of response body in httpRespToAPIResp which resulted in undefined behavior.

candysmurf · 2016-11-16T00:01:20Z

LGTM

thomastaylor312 reviewed Nov 2, 2016

View reviewed changes

thomastaylor312 approved these changes Nov 2, 2016

View reviewed changes

candysmurf suggested changes Nov 3, 2016

View reviewed changes

lmroz reviewed Nov 8, 2016

View reviewed changes

IRCody force-pushed the tribe_bug branch from 6446794 to ac70099 Compare November 8, 2016 18:02

andrzej-k reviewed Nov 15, 2016

View reviewed changes

IRCody force-pushed the tribe_bug branch from ac70099 to 5d8b904 Compare November 15, 2016 17:48

candysmurf suggested changes Nov 15, 2016

View reviewed changes

croseborough added 2 commits November 15, 2016 15:26

Fixes intelsdi-x#1324: Properly closes connections in tribe

7b23de9

Properly closes some http.Respons's that were not previously closed.

Avoid leaking http.Transports.

924cf8d

Avoids leaking http.Transports on client creation by re-using transports. Removes duplicate close of response body in httpRespToAPIResp which resulted in undefined behavior.

IRCody force-pushed the tribe_bug branch from 5d8b904 to 924cf8d Compare November 15, 2016 23:27

candysmurf approved these changes Nov 16, 2016

View reviewed changes

candysmurf merged commit c63df74 into intelsdi-x:master Nov 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1324: Properly closes connections in tribe #1327

Fixes #1324: Properly closes connections in tribe #1327

IRCody commented Nov 1, 2016

thomastaylor312 Nov 2, 2016 •

edited

Loading

IRCody Nov 2, 2016 •

edited

Loading

thomastaylor312 Nov 2, 2016

candysmurf Nov 3, 2016

lmroz Nov 3, 2016

candysmurf commented Nov 3, 2016 •

edited

Loading

candysmurf Nov 3, 2016

lmroz commented Nov 3, 2016 •

edited

Loading

lmroz commented Nov 3, 2016 •

edited

Loading

IRCody commented Nov 3, 2016

lmroz commented Nov 3, 2016

IRCody commented Nov 3, 2016

IRCody commented Nov 7, 2016

nanliu commented Nov 7, 2016 •

edited

Loading

geauxvirtual commented Nov 7, 2016

IRCody commented Nov 8, 2016

lmroz Nov 8, 2016

IRCody Nov 8, 2016

lmroz Nov 8, 2016

IRCody Nov 8, 2016

lmroz Nov 8, 2016

IRCody Nov 8, 2016

lmroz Nov 8, 2016 •

edited

Loading

thomastaylor312 commented Nov 10, 2016

andrzej-k Nov 15, 2016

IRCody Nov 15, 2016

andrzej-k Nov 15, 2016

IRCody commented Nov 15, 2016

candysmurf Nov 15, 2016

candysmurf Nov 15, 2016

IRCody Nov 15, 2016

candysmurf Nov 15, 2016

IRCody Nov 15, 2016

candysmurf commented Nov 16, 2016

Fixes #1324: Properly closes connections in tribe #1327

Fixes #1324: Properly closes connections in tribe #1327

Conversation

IRCody commented Nov 1, 2016

thomastaylor312 Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

IRCody Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candysmurf commented Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

lmroz commented Nov 3, 2016 • edited Loading

lmroz commented Nov 3, 2016 • edited Loading

IRCody commented Nov 3, 2016

lmroz commented Nov 3, 2016

IRCody commented Nov 3, 2016

IRCody commented Nov 7, 2016

nanliu commented Nov 7, 2016 • edited Loading

geauxvirtual commented Nov 7, 2016

IRCody commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmroz Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

thomastaylor312 commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IRCody commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candysmurf commented Nov 16, 2016

thomastaylor312 Nov 2, 2016 •

edited

Loading

IRCody Nov 2, 2016 •

edited

Loading

candysmurf commented Nov 3, 2016 •

edited

Loading

lmroz commented Nov 3, 2016 •

edited

Loading

lmroz commented Nov 3, 2016 •

edited

Loading

nanliu commented Nov 7, 2016 •

edited

Loading

lmroz Nov 8, 2016 •

edited

Loading