A few feedback items about rehttp HOT 11 CLOSED

Hello Justin,

Thanks for the feedback, this is very valuable information! There are some points that are probably down to API preferences, but I agree that having per-attempt timeouts (in addition to overall timeout) would be nice. Of course, this package was implemented a while ago before contexts were added to the stdlib, the implementation and API would certainly be different if I was to start over today (the approach to cancel a request has changed multiple times in Go's history).

I think the last point would still be out of scope for such a package (to me) if I was to start over, though. I think using a middleware approach with "the Doer" http client interface is a better/more flexible pattern (i.e. using a value that implements this interface:

type Doer interface {
   Do(req *http.Request) (*http.Response, error)
}

and then using middleware that wraps the Doer in a similar way to e.g. http.Handler).

I'm not planning on adding to this package in the near future, but if anyone seeing this is interested in implementing the deprecated Temporary check and even the per-attempt timeout support, I'd be happy to merge this (with tests, of course).

Thanks!
Martin

from rehttp.

Thanks for taking the time to read Martin. I’d be happy to take a crack at both of those. I’ll try to have something to review this week.

from rehttp.

Thanks for your contributions @justinrixx ! This is now merged and part of v1.3.0, I'll close this ticket now.

from rehttp.

Thanks for working with me on these. Although I use open-source software every day and work as a developer for a living, this has been my first experience actually making a pull request to an open source project 😅

There's just one more improvement I think would be beneficial, which is the ability to one-off override the transport's fields. I found this useful because generally I'll create an HTTP client for each of my service's dependencies, but the retry or delay policy or the per-attempt timeout may only make sense for most endpoints, but for just one or two endpoints they need to be overridden. To accomplish that with the current version of this package, each client would need to have multiple HTTP clients that each have their own roundtrippers with different configurations.

In my internal version, I accomplished this using context keys. If a context key is present, it takes precedence over the transport's configuration. I have several helper functions for setting various options on the context which take a context and the options to set, and return the resulting context.

For example, you might have something like:

type retryFnContextKeyType string

const retryFnContextKey retryFnContextKeyType("retryFn")

// this could be named something different like WithRetryFn(), etc
func SetRetryFnOnContext(ctx context.Context, retry RetryFn, delay DelayFn) context.Context {
	return context.WithValue(ctx, retryFnContextKey, toRetryFn(retry, delay))
}

// note that `retryFn` here is not the same as `RetryFn`. `retryFn` is a combination of `RetryFn` and `DelayFn`.
// this internal function is used inside `RoundTrip` to determine if the `retryFn` should be overridden.
func getRetryFnFromContext(ctx context.Context) (retryFn, bool) {
	return ctx.Value(retryFnContextKey).(retryFn)
}

This allows a consumer to do something like this:

tr := rehttp.NewTransport(
	nil, // will use http.DefaultTransport
	rehttp.RetryAll(rehttp.RetryMaxRetries(3), rehttp.RetryTemporaryErr()), // max 3 retries for Temporary errors
	rehttp.ConstDelay(time.Second),                                         // wait 1s between retries
)
client := &http.Client{
	Transport: tr,
}

func someEndpoint(ctx context.Context, requestURL string) (SomeType, error) {
	overrideContext := rehttp.SetRetryFnOnContext(
		ctx,
		rehttp.RetryAll(rehttp.RetryMaxRetries(1), rehttp.RetryTemporaryErr()), // max 1 retry
		rehttp.ConstDelay(time.Second * 5) // 5s instead of 1
	)

	req, err := http.NewRequest(http.MethodGet, requestURL, nil)
	if err != nil {
		...
	}
	res, err := client.Do(req.WithContext(overrideContext))
	// handle res and err
	...

I think this makes sense for setting the RetryFn + DelayFn pair, in addition to the per-attempt timeout and PreventRetryWithBody, but there might be more (you could possibly make the case for setting the internal roundtripper?).

I'm happy to make another pull request with those added if you agree that they're useful and state your preference on naming convention (SetRetryFnOnContext() vs WithRetryFn() vs something else). Naturally for go versions before 1.7 these options will have no effect, but I think that's ok as it'll just be a no-op and won't break anything.

from rehttp.

Hey Justin,

I'm not of fan of abusing the context to replace an API, but your proposal made me think of something that could be maybe a bit more general and similar to something that exists in the stdlib, what do you think if instead of relying on special keys in the context, we added a AttemptContext func(ctx context.Context) context.Context field to the Transport struct, much like the BaseContext and (most similarly) the ConnContext in the http.Server struct (https://pkg.go.dev/net/http#Server)?

In RoundTrip, we'd call that function if set just before the RoundTripper and use its context for the round trip.

There are still some details to figure out if we go that way:

• Does it receive the context with the configured PerAttemptTimeout already set? If so it can only generate a context with a shorter timeout (unless it creates a context that is not a child of the provided context), but if not, it renders the PerAttemptTimeout useless. I think it should be the former.
• What else should this function field receive as argument? The Request struct and the attempt number?
• It should probably return a cancel function along with the context.

That's what I had in mind so far!

Martin

from rehttp.

Thanks for the suggestions Martin.

I’m on vacation away from my workstation for a week, but I’ll read up on the references you made and put forward a proposal / prototype once I’ve returned.

from rehttp.

Just a couple clarifications to help me understand:

• Returning a context.Context is really only useful for setting the overall timeout, right? Your proposal doesn't allow for overriding other options like the RetryFn or per-attempt timeout. If I want to have a separate retry policy for one endpoint, my client struct is going to need to maintain a separate rehttp.Transport (and http.Client) specifically for that endpoint.
• You've suggested the name AttemptContext, implying it's meant to be public. How do you envision a consumer would use it? You don't intend a consumer to use it themselves, right? I think it'd be cleanest if it is only called from within RoundTrip. Essentially, right at the top, generate the context with the AttemptContext function (if provided), then use that context instead of the one originally on the request. I guess that's essentially how the per-attempt timeout is represented: it's got to be a public field in order to avoid breaking the constructor function API (I know this doesn't help the project at this stage, but this is another win for the Options pattern: new constructor options can be added without breaking the API).
• Putting on my consumer hat, I would want to change options based on the endpoint I'm hitting in the target service (url path). For that, I'd need the http.Request. There may be a use-case for passing in attempt number as well as you suggested, and possibly others neither of us have thought of. One backwards-compatible way to make that API is to have it accept a struct that can be augmented with additional fields later on down the road (AttemptForContext?).

As a consumer, my use-case of wanting a separate overall timeout for 2 or 3 endpoints in a client would have 2 options to solve:

• Add a 1-liner context.WithTimeout(parentCtx, desiredTimeout) in my client struct before the 2 or 3 endpoints that need to be overridden
• Provide an AttemptContext function similar to the below:

f := func(ctx context.Context, attempt rehttp.AttemptForContext) (context.Context, context.CancelFunc) {
   if matchesEndpoint1Regex(attempt.Req.URL.Path) {
      return context.WithTimeout(ctx, endpoint1Timeout)
   }
   if matchesEndpoint2Regex(attempt.Req.URL.Path) {
      return context.WithTimeout(ctx, endpoint2Timeout)
   }

   // no match, no-op
   return ctx, func() {}
}

I personally wouldn't choose option 2 over option 1. Maybe there's something I'm not understanding here, but at the moment I don't think this is worth pursuing.

from rehttp.

Hello Justin,

Returning a context.Context is really only useful for setting the overall timeout, right?

No, it would be for the per-attempt timeout (actually it would be for setting the context of the per-attempt request, so it could be used for whatever is needed for that attempt but typically I think that would be setting a per-attempt timeout). It would be called just before the round-trip.

Your proposal doesn't allow for overriding other options like the RetryFn

That's correct, but at this point, if you need a different per-attempt timeout and retry policies, why would you reuse that same rehttp transport?

You've suggested the name AttemptContext, implying it's meant to be public. How do you envision a consumer would use it? You don't intend a consumer to use it themselves, right?

Yes, I did envision consumers of rehttp to set it themselves much like the http.Server.BaseContext and http.Server.ConnContext fields are used. I'm not sure I understand the alternative you're proposing of something not publicly exposed? I guess you still mean using special keys in the provided context?

Add a 1-liner context.WithTimeout(parentCtx, desiredTimeout) in my client struct before the 2 or 3 endpoints that need to be overridden

How would that set a distinct per-attempt timeout while reusing you're rehttp transport? Or did you mean something closer to context.WithValue(ctx, rehttp.SpecialKeyForPerAttemptTimeout, desiredTimeout) before calling the client.Do for the request?

I think the difference in approach is that you feel strongly that rehttp should support that use-case of reusing the same transport for some URLs that don't follow the same settings as those set in the rehttp.Transport, without having to create a distinct rehttp.Transport and http.Client. I may be wrong, but I don't feel that's a very typical use-case, and what I'm trying to think of is a more general way of supporting it that could be useful for more use-cases.

I don't want to go down the route of adding hidden (as far as discovery via the exposed API) options in context keys to override the explicitly set configuration, so the AttemptContext function is in that spirit. It does mean you would have to do the per-URL check in that function to specify the proper context as return value, but then again you would have to do that check somewhere in your code anyway even if you were using special keys to override the timeout. It just offers a standard place to do that logic.

Let me know if that helps understanding what I was proposing!

Thanks,
Martin

from rehttp.

Hi Martin, sorry for the delay in my response.

No, it would be for the per-attempt timeout (actually it would be for setting the context of the per-attempt request, so it could be used for whatever is needed for that attempt but typically I think that would be setting a per-attempt timeout).

The per-attempt timeout is implemented as a context timeout derived from the request's context. So any timeout that already exists on the overall context will behave as an overall timeout regardless of the per-attempt timeout setting.

I'm not sure I understand the alternative you're proposing of something not publicly exposed? I guess you still mean using special keys in the provided context?

I think I was thinking of using something like func(ctx context.Context, attempt rehttp.Attempt) context.Context so that a different context can be produced for each attempt (would allow for per-attempt timeouts rather than just overall ones), but I'm just not certain how it would look either way.

How would that set a distinct per-attempt timeout while reusing your rehttp transport? Or did you mean something closer to context.WithValue(ctx, rehttp.SpecialKeyForPerAttemptTimeout, desiredTimeout) before calling the client.Do for the request?

I was originally thinking of just adding a new timeout, but you're right that can only override it to be more strict and not less strict (e.g. if the per-attempt timeout of the transport is set at 10s, I can use context.WithTimeout to set a 5s timeout, but if I try to set it to 15s it'll still be a 10s per-attempt timeout.)

I think the difference in approach is that you feel strongly that rehttp should support that use-case of reusing the same transport for some URLs that don't follow the same settings as those set in the rehttp.Transport, without having to create a distinct rehttp.Transport and http.Client.

Yes, that's my main complaint with this package as implemented currently. That said, I don't feel super strongly about it, it's just nice to have. I agree that it's borderline abuse of the context key api; where we differ is that I feel the benefits of that tradeoff outweigh the downsides and you don't and that's ok with me. Given that, I don't think it's worth pursuing that path any further.

I am still unclear on the benefits of using the AttemptContext besides as an overall timeout (independent of the per-attempt timeout).

from rehttp.

Hey Justin,

I agree that it's borderline abuse of the context key api; where we differ is that I feel the benefits of that tradeoff outweigh the downsides and you don't and that's ok with me. Given that, I don't think it's worth pursuing that path any further.

Ok, yeah that sums it pretty well, I agree with that. Just to clarify the last point you were wondering about (those two quotes from your comment seem related to the same misunderstanding):

I am still unclear on the benefits of using the AttemptContext besides as an overall timeout

The per-attempt timeout is implemented as a context timeout derived from the request's context. So any timeout that already exists on the overall context will behave as an overall timeout regardless of the per-attempt timeout setting.

I'll try to phrase it differently than in my last comment, but the AttemptContext field would've been a function that would've been called to derive a new context from the request's context. The link in that comment pointed to where that function would've been called, and the context returned from that function (and derived from the request's context) would've been used for the attempt. As such, whatever timeout that the AttemptContext function would've set, that timeout would've applied for the attempt.

To put this in (untested) code, today we have this in the RoundTrip implementation to set the per-attempt timeout:

		reqWithTimeout := req
		if t.PerAttemptTimeout != 0 {
			reqWithTimeout, cancel = getPerAttemptTimeoutInfo(ctx, req, t.PerAttemptTimeout)
		}

And with the AttemptContext addition, it would've looked like this:

		reqWithTimeout := req
		if t.PerAttemptTimeout != 0 {
			reqWithTimeout, cancel = getPerAttemptTimeoutInfo(ctx, req, t.PerAttemptTimeout)
		}

		if t.AttemptContext != nil {
			ctx = t.AttemptContext(ctx, /* other args, unclear which ones */)
			reqWithTimeout = req.WithContext(ctx)
		}

So as you can see, it would've set the per-attempt context timeout, not the overall one (and of course whichever timeout comes first would apply, as is the case for the PerAttemptTimeout field).

Anyway, not that it matters much but wanted to clarify this as you still thought this was used to set the overall timeout.

Martin

from rehttp.

Got it, that does make more sense to me now.

In any case, thanks for taking the time to discuss and merge my small contributions.

from rehttp.