Bypass request delay when request is cancelled #619
Conversation
| select { | ||
| case r.waitChan <- true: | ||
| case <-request.Context().Done(): | ||
| return nil, errors.New("context canceled; early return") |
There was a problem hiding this comment.
I believe earlier version of this PR returned ctx.Err() instead. Why did you decide to return a new error instead?
There was a problem hiding this comment.
Yes, that was the case. Usually i would use wrap from github.com/pkg/errors and just add to the error but the library is not in use here.
My first try at writing the code I was asserting against the time that it took to run all the calls and check if they return asap after the cancel; worked fine locally but fail on CI and yea, check like that is really unreliable.
So my second attempt its based on the error message; to check if the error was the normal error from the request or if it was the early return; I had to make the early return message different from ctx.Err() cause that is what we get from the http client that tries to run with a cancelled request.
There was a problem hiding this comment.
Sorry for the late response. I don't think tests are good enough reason to return different errors (depending on where it happened) when context is cancelled. Are you aware of any other library that does not return ctx.Err() unchanged?
There was a problem hiding this comment.
No worries, any suggestions on how to get the test done?
There was a problem hiding this comment.
My first try at writing the code I was asserting against the time that it took to run all the calls and check if they return asap after the cancel; worked fine locally but fail on CI and yea, check like that is really unreliable.
I suppose you could make a test like this:
- Make the test HTTP handler sleep before returning the response, maybe a second or more.
- Set low rate limit, say, 1 req/second.
- Spawn lots of goroutines. Doesn't have to be too much, though, maybe 20-30 or so is enough.
- Cancel context.
- Assert that cancellation does happen in 5 seconds. This value seems high enough so the test would be reliable, but low enough so the test won't accidentally pass without the fix.
What do you think?
http_backend_test.go
Outdated
| ) | ||
|
|
||
| func TestHTTPBackendDoCancelation(t *testing.T) { | ||
| rand.Seed(time.Now().Unix()) |
There was a problem hiding this comment.
It's better to not have any indeterminate behaviour in tests. Either just make make p, n, and c some constants, or, if you're unsure, use math/rand.New() with some constant seed and run your test multiple times using the same generator.
There was a problem hiding this comment.
Made a commit with a hardcoded seed. :)
There was a problem hiding this comment.
I'd still advise you to use rand.New(). Less global state - the better.
Before this PR: after cancelling a context for a collector all requests failed with "context canceled" but the delay set by the limit rules still applies.
The intention here is to terminate the collector as soon as possible after its context finishes.