Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562293002:
common/lhttp: close the Response.Body on non-200s (Closed)
|
Created:
4 years ago by djd-OOO-Apr2017 Modified:
4 years ago CC:
chromium-reviews, infra-reviews+luci-go_chromium.org, maruel+w_chromium.org, tandrii+luci-go_chromium.org Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
Descriptioncommon/lhttp: close the Response.Body on non-200s
Refactor lhttp NewRequest to make sure that the response's body is
always drained/closed in the case that the handler is not invoked (for
5xx responses, etc.).
Add text to the returned error from NewRequest to make it how many
attempts were attempted before failing.
BUG=671592
Review-Url: https://codereview.chromium.org/2562293002
Committed: https://github.com/luci/luci-go/commit/64707122e72f7405ce9b04b38323cf8bbf7cbf2d
Patch Set 1 #Patch Set 2 : fix typos #
Total comments: 7
Patch Set 3 : Reorder struct fields #
Dependent Patchsets: Messages
Total messages: 20 (10 generated)
djd@chromium.org changed reviewers: + mcgreevy@chromium.org
djd@chromium.org changed reviewers: + maruel@chromium.org
Ugh thanks. Only one style nit. https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... File common/lhttp/client_test.go (right): https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... common/lhttp/client_test.go:285: if got, want := status, http.StatusOK; got != want { That's an unusual style (to me) but it's fine. In general I use the words "actual" and "expected", although they are longer to type and bytes are pricey. https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... common/lhttp/client_test.go:316: mu sync.Mutex switch the order to make it clear what mu protects.
On 2016/12/12 17:16:39, M-A Ruel wrote: > Ugh thanks. I meant lgtm, duh. :) > Only one style nit. > > https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... > File common/lhttp/client_test.go (right): > > https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... > common/lhttp/client_test.go:285: if got, want := status, http.StatusOK; got != > want { > That's an unusual style (to me) but it's fine. > > In general I use the words "actual" and "expected", although they are longer to > type and bytes are pricey. > > https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... > common/lhttp/client_test.go:316: mu sync.Mutex > switch the order to make it clear what mu protects.
https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... File common/lhttp/client_test.go (left): https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... common/lhttp/client_test.go:128: ut.AssertEqual(t, true, errors.IsTransient(err)) Why are you removing these IsTransient checks?
https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... File common/lhttp/client_test.go (left): https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... common/lhttp/client_test.go:128: ut.AssertEqual(t, true, errors.IsTransient(err)) On 2016/12/12 23:57:39, mcgreevy2 wrote: > Why are you removing these IsTransient checks? I've changed Retry so that it now wraps the final error to add the "attempts" suffix. AFAICT, the fact that the last error was transient is not important for the caller of Retry so I was okay removing it. https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... File common/lhttp/client_test.go (right): https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... common/lhttp/client_test.go:285: if got, want := status, http.StatusOK; got != want { On 2016/12/12 17:16:39, M-A Ruel wrote: > That's an unusual style (to me) but it's fine. > > In general I use the words "actual" and "expected", although they are longer to > type and bytes are pricey. I agree. I prefer the work "actual" much more than "got" (I have a bit of a personal vendetta against that awful word). However the "got/want" is the prevailing Go style and used consistently throughout the community: https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures (I can find even better resources internally if you're interested). If you prefer, I can migrate the rest of the tests to be consistent? Let me know what you think. https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... common/lhttp/client_test.go:316: mu sync.Mutex On 2016/12/12 17:16:39, M-A Ruel wrote: > switch the order to make it clear what mu protects. Done.
The CQ bit was checked by djd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... File common/lhttp/client_test.go (right): https://codereview.chromium.org/2562293002/diff/20001/common/lhttp/client_tes... common/lhttp/client_test.go:285: if got, want := status, http.StatusOK; got != want { On 2016/12/13 00:18:09, djd wrote: > On 2016/12/12 17:16:39, M-A Ruel wrote: > > That's an unusual style (to me) but it's fine. > > > > In general I use the words "actual" and "expected", although they are longer > to > > type and bytes are pricey. > > I agree. I prefer the work "actual" much more than "got" (I have a bit of a > personal vendetta against that awful word). However the "got/want" is the > prevailing Go style and used consistently throughout the community: > https://github.com/golang/go/wiki/CodeReviewComments#useful-test-failures > (I can find even better resources internally if you're interested). > > If you prefer, I can migrate the rest of the tests to be consistent? Let me know > what you think. That's totally bikeshedding level at that point, I really don't care. :) Let's move on and be productive. That said, I love that the immediate sentence after the code snippet says: "Note that the order here is actual != expected, and the message uses that order too" as in: "The words we meant are 'actual' and 'expected' but we use something else just to mess with you. Ahah we're funny."
The CQ bit was checked by djd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2562293002/#ps40001 (title: "Reorder struct fields")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481595003819890,
"parent_rev": "c7aa7c4c3dd84735f2ac1e386a26677b774ac996", "commit_rev":
"64707122e72f7405ce9b04b38323cf8bbf7cbf2d"}
Message was sent while issue was closed.
Description was changed from ========== common/lhttp: close the Response.Body on non-200s Refactor lhttp NewRequest to make sure that the response's body is always drained/closed in the case that the handler is not invoked (for 5xx responses, etc.). Add text to the returned error from NewRequest to make it how many attempts were attempted before failing. BUG=671592 ========== to ========== common/lhttp: close the Response.Body on non-200s Refactor lhttp NewRequest to make sure that the response's body is always drained/closed in the case that the handler is not invoked (for 5xx responses, etc.). Add text to the returned error from NewRequest to make it how many attempts were attempted before failing. BUG=671592 Review-Url: https://codereview.chromium.org/2562293002 Committed: https://github.com/luci/luci-go/commit/64707122e72f7405ce9b04b38323cf8bbf7cbf2d ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://github.com/luci/luci-go/commit/64707122e72f7405ce9b04b38323cf8bbf7cbf2d |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
