store: handle EOF via url.Error check #3126

Merged
merged 8 commits into from Apr 5, 2017

Conversation

Projects
None yet
4 participants
Contributor

stolowski commented Apr 3, 2017

Handle EOF errors such as "Post http://127.0.0.1:36904/updates/: EOF" by introspection via url.Error. Added test cases for EOF.

mvo5 approved these changes Apr 3, 2017

Looks good, thanks for tracking this done! Some small suggestions inside but no blockers.

store/store_test.go
+ mockServer.CloseClientConnections()
+ return
+ }
+ jsonReq, err := ioutil.ReadAll(r.Body)
@mvo5

mvo5 Apr 3, 2017

Collaborator

You could write this slightly shorter by using a json.Decoder, something like:

var resp struct {
			Snaps  []map[string]interface{} `json:"snaps"`
			Fields []string                 `json:"fields"`
		}
		err := json.NewDecoder(r.Body).Decode(&resp)
		c.Assert(err, IsNil)
@stolowski

stolowski Apr 3, 2017

Contributor

Right, good idea, done.

+ c.Assert(mockServer, NotNil)
+ defer mockServer.Close()
+
+ var err error
@mvo5

mvo5 Apr 3, 2017

Collaborator

This looks unneeded.

@stolowski

stolowski Apr 3, 2017

Contributor

I've simplified it a bit, there is no need to examine all that in detail indeed (we have other tests that do that), but I think a sanity test that ensure the client sends seinsible data on retry is useful (this gives us confidence that subsequent requests have proper body after initial failure). Let me know if that is what you meant here?

@zyga

zyga Apr 4, 2017

Contributor

I think what mvo meant is that you don't need to define it as the subsequent line does it for you.

@stolowski

stolowski Apr 4, 2017

Contributor

Ah, right, thanks for pointing out. Fixed.

stolowski added some commits Apr 3, 2017

mvo5 approved these changes Apr 3, 2017

zyga and others added some commits Apr 4, 2017

LGTM, but let's please tweak the logic just slightly:

store/retry.go
@@ -70,6 +71,9 @@ func shouldRetryError(attempt *retry.Attempt, err error) bool {
if !attempt.More() {
return false
}
+ if urlErr, ok := err.(*url.Error); ok {
+ return urlErr.Err == io.ErrUnexpectedEOF || urlErr.Err == io.EOF
@niemeyer

niemeyer Apr 4, 2017

Contributor

Instead of repeating the check here, it'd be better to just unwrap and let the follow up code run

if urlErr, ok := err.(*url.Error); ok {
        err = urlErr
}
@stolowski

stolowski Apr 4, 2017

Contributor

Good idea, done, althouth it needs to be urlErr.Err, and also the follow up condition on netErr needs "else if", otherwise the re-assigned err falls into it and into Timeout() check.

stolowski added some commits Apr 4, 2017

Collaborator

mvo5 commented Apr 5, 2017

@stolowski Thanks for addressing gustavos point!

@mvo5 mvo5 merged commit 26dbeba into snapcore:master Apr 5, 2017

6 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-ppc64el autopkgtest finished (success)
Details
yakkety-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment