kamilsk / retry

♻️ The most advanced interruptible mechanism to perform actions repetitively until successful.
https://pkg.go.dev/github.com/kamilsk/retry/v5
MIT License
340 stars 14 forks source link

Preserve context values #158

Closed bojan-embroker closed 3 years ago

bojan-embroker commented 3 years ago

retry.Do doesn't preserve context values when calling action. Here is an example:

ctx := context.WithValue(context.Background(), "asdf", 3)
action := func(ctx context.Context) error {
    log.Println(ctx.Value("asdf")) // Doesn't print 3
    return nil
}

retry.Do(ctx, action)

Does it make sense to change retry.Do to preserve context values?

kamilsk commented 3 years ago

Hi! Yes, retry.Do doesn't preserve context values when calling action because it expects strategy.Breaker, not context.Context.

It is possible to do that:

-   ctx, cancel := context.WithCancel(context.Background())
+   ctx, is := breaker.(context.Context)
+   if !is {
+       ctx = context.Background()
+   }
+   ctx, cancel := context.WithCancel(ctx)

I'm not a fan of the Value part of the context, but could you provide some use cases to make a decision.

bojan-embroker commented 3 years ago

Hi Kamil,

I am used to storing correlation id in context so it gets prepended to all log entries. For instance when I am building a REST API, I use request id as correlation id and I can then easily inspect all log entries produced by a request even if they are interleaved with other entries. That's how I found out retry.Do isn't preserving values - correlation id was missing from some log entries. There are other ways to do it of course, but projects I work on do it like that.

Database transaction handle also gets stored in context.

kamilsk commented 3 years ago

Thanks, Bojan! I'm preparing the patch and will add some tests to it.

Today it will be ready, the upcoming release is v5.0.0-rc6.

bojan-embroker commented 3 years ago

Thank you very much!

kamilsk commented 3 years ago

https://github.com/kamilsk/retry/releases/tag/v5.0.0-rc6