Create packages client/internal/ retry and lhttp.

Create packages client/internal/ retry and lhttp.

- Migrate HTTP related code from client/internal/common to lhttp.
- Add automatic retry function on HTTP failure via package retry.

The retry package has 100% test coverage and lhttp in the 90%.

It will be the building block to add authentication.

R=vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/luci-go/commit/3b1e91d92068d9e76409d00484d3d47fc40ed3cf

[M-A Ruel, 2015-05-13 19:24:22]

Patchset #1 (id:1) has been deleted

[M-A Ruel, 2015-05-13 19:24:44]

Patchset #1 (id:20001) has been deleted

[M-A Ruel, 2015-05-13 19:26:28]

maruel@chromium.org changed reviewers:
+ vadimsh@chromium.org

[M-A Ruel, 2015-05-13 19:32:08]

Patchset #1 (id:40001) has been deleted

[M-A Ruel, 2015-05-13 19:32:49]

I expect this to be the basis of all future http client code so I hope you like
it. It has the interesting property that it can be used for long lived HTTP
requests just fine.

[Vadim Sh., 2015-05-13 21:54:48]

lgtm with nits

looks neat :)

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
File client/internal/lhttp/client.go (right):

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
client/internal/lhttp/client.go:81: // Retriable.
ideally JSON formatting errors and network related errors should be
distinguished somehow :( Stupid Go makes it difficult.

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
client/internal/lhttp/client.go:106: // NewReader returns a io.ReadCloser
compatible read-only buffer that also
I think it's better to keep it private. lhttp/client is not an obvious place to
find this sort of functions, so no need to expose it here.

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
client/internal/lhttp/client.go:156: if strings.HasPrefix(err.Error(),
"unsupported protocol scheme") {
that's what I meant when I was saying "stupid go" :)

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
client/internal/lhttp/client.go:166: if resp.StatusCode >= 400 {
I may make sense to return ErrNotFound on 404 and ErrNotAuthorized on 401 and
403.

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/d...
File client/internal/lhttp/doc.go (right):

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/d...
client/internal/lhttp/doc.go:5: // Package lhttp implements HTTP client helper
code (JSON, automatic retries,
nit: explain that 'l' is for "LUCI" :)

https://codereview.chromium.org/1135173003/diff/60001/client/internal/retry/d...
File client/internal/retry/doc.go (right):

https://codereview.chromium.org/1135173003/diff/60001/client/internal/retry/d...
client/internal/retry/doc.go:7: // retry.Error type to knowwhen an error should
be retried.
typo: knowwhen

https://codereview.chromium.org/1135173003/diff/60001/client/isolatedclient/i...
File client/isolatedclient/isolatedclient.go (right):

https://codereview.chromium.org/1135173003/diff/60001/client/isolatedclient/i...
client/isolatedclient/isolatedclient.go:65: return lhttp.PostJSON(retry.Default,
http.DefaultClient, i.url+resource, in, out)
let's put http.Client and retry.Config into isolateServer instead of using
defaults here. Same for Swarming below.

[M-A Ruel, 2015-05-14 01:00:54]

I created a new interface lhttp.Retriable. You can diff patchset #1 and #2 to
see the difference.

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
File client/internal/lhttp/client.go (right):

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
client/internal/lhttp/client.go:81: // Retriable.
On 2015/05/13 21:54:47, Vadim Sh. wrote:
> ideally JSON formatting errors and network related errors should be
> distinguished somehow :( Stupid Go makes it difficult.

Actually, you just create typed errors. That's exactly what I do with
retry.Error. The problem is the URL parsing that can fail but I added code to
check it earlier instead.

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
client/internal/lhttp/client.go:106: // NewReader returns a io.ReadCloser
compatible read-only buffer that also
On 2015/05/13 21:54:47, Vadim Sh. wrote:
> I think it's better to keep it private. lhttp/client is not an obvious place
to
> find this sort of functions, so no need to expose it here.

Ok but it's possible we may need to expose it for non JSON posts.

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
client/internal/lhttp/client.go:156: if strings.HasPrefix(err.Error(),
"unsupported protocol scheme") {
On 2015/05/13 21:54:47, Vadim Sh. wrote:
> that's what I meant when I was saying "stupid go" :)

Removed since I added a check earlier. It's only an issue because the error type
is wrapped.

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/c...
client/internal/lhttp/client.go:166: if resp.StatusCode >= 400 {
On 2015/05/13 21:54:47, Vadim Sh. wrote:
> I may make sense to return ErrNotFound on 404 and ErrNotAuthorized on 401 and
> 403.

Changed to add an interface instead if needed. The interface exposes Status(). I
had started using an interface initially, removed it and now added it back. It
permits adding more stateful information.

For the JSON functions, I just added status as a return code.

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/d...
File client/internal/lhttp/doc.go (right):

https://codereview.chromium.org/1135173003/diff/60001/client/internal/lhttp/d...
client/internal/lhttp/doc.go:5: // Package lhttp implements HTTP client helper
code (JSON, automatic retries,
On 2015/05/13 21:54:48, Vadim Sh. wrote:
> nit: explain that 'l' is for "LUCI" :)

Done.

[Vadim Sh., 2015-05-14 01:27:59]

lgtm

[M-A Ruel, 2015-05-15 01:41:10]

Committed patchset #2 (id:80001) manually as
3b1e91d92068d9e76409d00484d3d47fc40ed3cf (presubmit successful).