Fixes retry support to not fail due to body not being rewound between retry attempts. (#204+#196)

[jasdel]

Fixes retry support to not fail due to body not being rewound between retry attempts.

Merged #196 and #204 together along with other changes to make sure rewind works in all cases, Made sure the ordering of handler's setting Request.Retryable vs the Service.ShouldRetry are consistent. A Request Handler setting Request.Retryable will prevent RetryHandler calling Service.ShouldRetry. The Service's delay value wills still be applied though.

Verified all tests pass and used @josharian's script to ping for aws request limits to verify retries were handled correctly.

Please try this out this PR and let me know if you hit retry failures still.

[jasdel]

git fetch upstream pull/211/head:PR_211_fixRetries
git checkout PR_211_fixRetries

where upstream is the remote: github.com/awslabs/aws-sdk-go, and PR_211_fixRetries is the local branch name to checkout this PR to.

[josharian]

LGTM. I'd be tempted to go for an int instead of SettableBool -- see e.g. https://github.com/golang/tools/blob/master/cmd/vet/main.go#L56 -- but it is minor, and immaterial to the question at hand.

It'll take a bit to get this integrated into our broader system, but if we still encounter problems then, I will diagnose...so full speed ahead!

Thanks so much for the help with this. I look forward to the merge. :)

[lsegal]

I also like the idea of the triState pattern. Looks like we could use that more broadly in our config options too!

[euank]

Any reason for preferring SettableBool or triState over just a default nil *bool?

The reason for triState in the cli tool, based on the comment, is that flag.Bool does not distinguish, but since this isn't a command line flag parsed with the flag package I see no reason not to just use a nillable bool. That also, imo, makes it easier for someone unfamiliar with the api otherwise to reason about / understand how to deal with the type.

[josharian]

*bool requires an alloc. Also, accidentally copying a *bool can make different values alias.

[euank]

@josharian The 'Set' method requires a pointer receiver to mutate it, and so you get an allocation for using the given struct as well, right? See playground.

With *bool, your editor will tell you about the type and you know you have to de-reference it to compare. You know it can be nil. With tristate, you have to know about the third state or you'll have logical errors (if true else /* false */ type).

That's my reasoning for why I'd prefer *bool anyways.

Edit: I realized that you probably meant the tirState option doesn't require an alloc. Oops, my misunderstanding! I'll still argue for something I feel is more understandable and type-safe over the cost of an alloc.

[jasdel]

The biggest argument I could see against using a pointer versus a type which knows if its wrapping value was set, is that with a pointer you must do nil checks before using the type's value.

I don't think we plan on using triState itself from flag, but the idea I think is very useful in our config state. The idea we are thinking that a TriStateBool Get() will return either the zero value, or value set. If the user of TriStateBool cares if the value was actually set IsSet() can be used.

It is my understanding that a pointer receiver means the object reference will be used and pointer to it will be provided to the function. Whereas not using a pointer receiver a copy of the receiver is made prior to being given to the function. This is why modifying a structure's property with a value receiver will not update the original structure's property the way you'd expect. e.g: http://play.golang.org/p/mNQxoUjGkM

[josharian]

Glad to see this merged! Thanks, @jasdel.

FWIW, @euank you don't use a Set method, you just assign a new value: http://play.golang.org/p/ZaGvE-9y_A. Tristate is pretty type-safe--attempting to use a Tristate where a bool is required will fail to compile.

[euank]

@josharian Yeah, per my edit I realized you meant tristate while typing my first paragraph and that my comment didn't actually apply. Sorry for the confusion there 😕

I recognize that it's type safe, but I still am concerned about the possibility for logic errors. If I type request.Retriable. and see my editor suggest the function .Bool() bool, I'm likely to not realize that there's a third value and not handle it.

Because there's not a language-level construct of Maybe<T> or similar, I can't be expected to be familiar with this construct already so I think such a mistake is quite possible.

On the other hand, nil and pointers are core to the language and a typical Go developer will understand that a pointer might be nil, thus making logical errors far less likely.

@jasdel The pointer requires nil checks, which I see as an argument for it, not against it. The tristate masks the third value somewhat, so you don't have to check for it, but you also might not realize you should check for it, leading to logically incorrect code.

I do see the argument for tristate though and my opposition is only minor.
Thanks for the discussion!