api: fix data race in Client.AddLocalCharm #2618

Merged
merged 1 commit into from Jun 22, 2015

Conversation

Projects
None yet
3 participants
Contributor

davecheney commented Jun 22, 2015

Updates LP 1465115

A data race exists because the Client holds a reference to the charm bundle file descriptor and closes it unconditionally on the way out of the method.
However, the net/http Client also has a reference to the file descriptor via the http request body and also closes it if the upload is successful.
It is not clear if the body will be closed if the request is not successful.

This creates a race on the Close method which is not safe to be called by multiple goroutines. Work around this by inserting a wrapper which nullifies the Close method presented to the http Client.

(Review request: http://reviews.vapour.ws/r/1992/)

api: fix data race in Client.AddLocalCharm
Updates LP 1465115

A data race exists because the Client holds a reference to the charm bundle file descriptor and closes it unconditionally on the way out of the method.
However, the net/http Client also has a reference to the file descriptor via the http request body and also closes it if the upload is successful.
It is not clear if the body will be closed if the request is not successful.

This creates a race on the Close method which is not safe to be called by multiple goroutines. Work around this by inserting a wrapper which nullifies the Close method presented to the http Client.
Contributor

mjs commented Jun 22, 2015

yuck. nice solution though.
LGTM

Contributor

davecheney commented Jun 22, 2015

$$merge$$

Contributor

jujubot commented Jun 22, 2015

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Jun 22, 2015

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/3739

Contributor

davecheney commented Jun 22, 2015

$$merge$$

Contributor

jujubot commented Jun 22, 2015

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

Contributor

jujubot commented Jun 22, 2015

Build failed: Generating tarball failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/3740

Contributor

davecheney commented Jun 22, 2015

$$merge$$

Contributor

jujubot commented Jun 22, 2015

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

jujubot added a commit that referenced this pull request Jun 22, 2015

Merge pull request #2618 from davecheney/fixedbugs/1465115
api: fix data race in Client.AddLocalCharm

Updates LP 1465115

A data race exists because the Client holds a reference to the charm bundle file descriptor and closes it unconditionally on the way out of the method.
However, the net/http Client also has a reference to the file descriptor via the http request body and also closes it if the upload is successful.
It is not clear if the body will be closed if the request is not successful.

This creates a race on the Close method which is not safe to be called by multiple goroutines. Work around this by inserting a wrapper which nullifies the Close method presented to the http Client.

@jujubot jujubot merged commit 2e038f4 into juju:master Jun 22, 2015

@davecheney davecheney deleted the davecheney:fixedbugs/1465115 branch Jun 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment