Add ability to set a per-attempt timeout #8
Conversation
justinrixx
force-pushed
the
per-attempt-timeouts
branch
from
4978d02
to
88f1256
Compare
|
just rebased to latest master |
rehttp.go
Outdated
| @@ -280,6 +282,21 @@ type Transport struct { | |||
| // is non-nil. | |||
| PreventRetryWithBody bool | |||
|
|
|||
| // PerAttemptTimeout can be optionally set to change the timeouts to be | |||
| // per-attempt instead of overall. For example, a per-attempt timeout of | |||
There was a problem hiding this comment.
It doesn't "change" the timeout to be per-attempt, right? It sets a per-attempt timeout, potentially in addition to the overall timeout as I understand it?
rehttp.go
Outdated
| res, err := t.RoundTripper.RoundTrip(req) | ||
| var cancel context.CancelFunc = func() {} // empty unless a timeout is set | ||
| reqWithTimeout := req | ||
| if t.PerAttemptTimeout != time.Duration(0) { |
There was a problem hiding this comment.
I'd simplify to this:
| if t.PerAttemptTimeout != time.Duration(0) { | |
| if t.PerAttemptTimeout != 0 { |
rehttp_server_post17_test.go
Outdated
|
|
||
| func TestTransport_RoundTripTimeouts(t *testing.T) { | ||
| attemptCountMu := sync.Mutex{} | ||
| attemptCount := 0 |
There was a problem hiding this comment.
I'd simplify this a bit by using an int32 and the atomic package, e.g. https://pkg.go.dev/sync/[email protected]#AddInt32
There was a problem hiding this comment.
If I use a buffered channel for synchronization (to eliminate the sleep) each of these will already effectively assert 4 attempts (because the test won't move on until 4 channel reads). That could totally eliminate the need for the attemptCount. Downside is that if this test ever breaks it'll break by hanging (waiting on a channel read), which won't make the failure mode obvious. Do you have a preference between the two? (eliminating attemptCount, its increments, and any assertions on it vs keeping it around but using atomic addition so that an assertion can fail with a clearer message before the channel reads?
There was a problem hiding this comment.
Actually keeping attemptCount doesn't make sense because if its value is checked before the channel reads the attempt count isn't final yet (server hasn't served them all). Checking its value after the channel reads is unhelpful because the channel reads already assert the correct number of attempts.
rehttp_server_post17_test.go
Outdated
| func(attempt Attempt) bool { // retry context deadline exceeded errors | ||
| return attempt.Error != nil && attempt.Error == context.DeadlineExceeded // errors.Is requires go 1.13+ | ||
| })), | ||
| ConstDelay(time.Duration(0)), |
There was a problem hiding this comment.
I don't think you need the type conversion here, I think the const will take the proper type:
| ConstDelay(time.Duration(0)), | |
| ConstDelay(0), |
rehttp_server_post17_test.go
Outdated
| } | ||
|
|
||
| _, err = client.Do(req) | ||
| time.Sleep(time.Millisecond * 100) // let the server finish sleeping |
There was a problem hiding this comment.
Not a fan of having sleeps in there as this is so often a source of flaky tests, but this might get a bit tricky to do otherwise (e.g. with a channel, waiting for it to be closed here). I'm fine keeping it as-is if the alternative is too complex.
| t.Errorf("status code does not match expected: got %d, want %d", res.StatusCode, http.StatusTooManyRequests) | ||
| } | ||
| attemptCountMu.Unlock() | ||
| } |
There was a problem hiding this comment.
What I think would be good to have is a test with both a per-attempt and an overall timeout, and a case where the overall triggers instead of the per-attempt?
There was a problem hiding this comment.
Added this in TestTransport_RoundTripOverallTimeout
rehttp_server_post17_test.go
Outdated
| // preventing the race-case of being unable to read the body due to a | ||
| // preemptively-canceled context. | ||
| func TestCancelReader(t *testing.T) { | ||
| rt := NewTransport(http.DefaultTransport, RetryMaxRetries(1), ConstDelay(time.Duration(0))) |
There was a problem hiding this comment.
Same here, time.Duration conversion probably not needed.
| } | ||
| if res == nil { | ||
| t.Error("unexpected nil response") | ||
| } |
There was a problem hiding this comment.
I don't think client.Do will ever return nil if err != nil? I think those res==nil or res != nil checks can savely be removed.
All reasonable feedback, I’ll try to get this revised over the next week or so. |
justinrixx
force-pushed
the
per-attempt-timeouts
branch
from
88f1256
to
32fc595
Compare
|
Hmm these tests passed locally so I thought it was ok. Might be flakiness with the per-attempt + overall timeout test making fewer or more attempts than there are channel slots. I'll take a look at revising these soon. |
justinrixx
force-pushed
the
per-attempt-timeouts
branch
from
32fc595
to
1b9ff12
Compare
|
I believe I've addressed all of your comments now. Let me know if I've missed something or if you have any other feedback. |
|
Thanks! I'll try to take another look this weekend to get it merged. |
| ch := make(chan bool, 4) | ||
|
|
||
| ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| ch <- true |
There was a problem hiding this comment.
The problem with this approach is that the test could block forever (well, until go test times out) if the assumptions are not met, instead of failing fast (i.e. if the client happens to make 5 requests, the 5th one would block forever because the channel is full and no receives have been done yet).
I have an idea on how to make the test more robust, I'll try to take a shot at it when I have a moment, the general idea would be to spin a goroutine that receives in a for range on the channel, and close the channel after the call to client.Do so that the goroutine exits, and unblocks another channel (or a WaitGroup, but the nice thing with a channel is that the test could do a select on that channel and a timer so that it fails fast if for some reason it takes longer than expected to run the test).
That's not super easy to explain, that's why I say I'll take a shot at implementing it when I have a moment.
There was a problem hiding this comment.
I'm having a hard time understanding exactly what you're describing, but I agree that if this does fail it ends up in the undesirable state of blocking on the channel until the test times out. I don't have a great idea for solving this, but will take a look at your change set to hopefully learn something when you add it in.
Thanks for spending the time to read, review, and re-review this.
| t.Errorf("got unexpected error doing request: %s", err) | ||
| } | ||
| if res == nil || res.StatusCode != http.StatusTooManyRequests { | ||
| t.Errorf("status code does not match expected: got %d, want %d", res.StatusCode, http.StatusTooManyRequests) |
There was a problem hiding this comment.
This will panic if res is nil. I suggest calling t.Fatalf on line 68 above, so that after this condition res is necessarily not nil. (I can take care of this change when I implement the channel thing I mentioned above).
|
Actually, it's just an improvement in the test, the feature looks good so I'll make the minor adjustments I mentioned and get it merged, we can always make the test more robust later on. I don' t know when I'll have time to get back to this and I don't want to leave your great work pending just for that. |
See #6
I haven't done any testing for go 1.6, but as called out in this change set setting a per-attempt timeout on go < 1.7 should be a no-op.
I also haven't tested this using go 1.7 (I'm running go 1.21), but I did my best to check the go versions the context and http request functions were added in and believe it's correct.
LMK if the cancel reader stuff doesn't make sense, I'm happy to further explain why it's necessary and / or add more to the docs (although it shouldn't matter at all to external consumers).