Investigate Best Practices with HTTP2 about hedgedhttp HOT 7 CLOSED

I have tested 0.4.0 on a http2 connection to www.google.com and with the gcs client. Worked like a charm with both.

Thank you for the work on this 🙏

from hedgedhttp.

Thank you for filling this issue, I'm not a big pro of net/http internals, so cannot answer so quickly, that's why I'm not sure about which path is the best.

There is #6 which removes context cancelling at all (we just use original context inside request, without creating own), but also there is a sub-context in closure for runInPool and maybe the problem will be triggered there.

Sadly I don't have good bench/project/whatever where I can test it under real load (btw, can you share how much requests do you make when this problem appears?) and trying to debug in head and docs.

from hedgedhttp.

Also, jumping through the link from original PR I've found a thread on Google Groups, slightly changing the code I got something like this https://play.golang.org/p/iV2UMxZGPtr (you need to run it locally, url doesn't matter, feel free to change).

Locally this 'sometimes' appears to be 'always' 👀

from hedgedhttp.

I like the change in #6, but I think that we will need an optional cancel like was originally proposed. After you merge #6 I'll submit a PR for the optional cancel.

from hedgedhttp.

@joe-elliott idea is to cancel the context outside of the hedgehttp.

ctx, cancel := context.WithCancel(otherCtx)
defer cancel()

req := <create desired request>
req = req.WithContext(ctx)

// do hedgedhttp request
// read the body if it's present

// invoke cancel() here

In this way we're getting response, reading the body, canceling a context (it's scope is just to contain hedging), had a discussion with @storozhukBM yesterday, it may be slightly sub-optimal because we're not closing hanging requests as fast as possible, but it's a matter of 'how long do you process' the successfully received response.

I'm super eager to check how does #6 behaves in your situation and if it doesn't fix it, we can add optional cancelling or even cancelling failed requests manually (more code and more responsibility).

from hedgedhttp.

So, https://github.com/cristalhq/hedgedhttp/releases/tag/v0.3.0 is available, please test it 😉

from hedgedhttp.

So, #9 has landed. Please try it https://github.com/cristalhq/hedgedhttp/releases/tag/v0.4.0 :)

from hedgedhttp.