kyash / async-retry Goto Github PK

Problem

Immediately after submitting the first PR, I realized that there is a fairly small chance of failure of graceful shutdown. This is very small chance, but we must fix this, I think.
The scenario looks like this:

1. call asyncRetry.Shutdown
2. call go func() { if err:=asyncRetry.Do(ctx,retryFunc); err!=nil { log.Error(err.Error())}}() in the other goroutine simultaneously
3. Before, 2'new goroutine is launched, 1 succeeded
4. 2 get InShutdownErr and try to log
5. Before logging, the instance this program running is completely shutdown(this is possible because Shutdown returned success already)
6. No chance to catch error message

Solution

I suggest two solutions.
Do you have any other good solutions?
If I had to choose A or B, I think we should choose A.

A

Do launches a new goroutine by itself after inclement a.wg, and interface is like Do(ctx context.Context, f AsyncRetryFunc, func(error), opts ...Option) . Caller calls like:

if err := asyncRetry.Do(
    ctx,
    retryFunc,
    func(err error){
        if err!=nil {
            log.Error(err.Error())
       }
     }
); err {
   // in shutdown case
}

• Pros
  • simple. easy to understand
  • always safe
• Cons
  • Interface is not sophisticated?
  • I prefer to goroutine be made explicitly at client code rather than made implicitly in the library

B

Change interfaces like:

type AsyncRetry interface {
         // increment a.wg
	Reserve() (Retryer, error)
	Shutdown(ctx context.Context) error
}

type Retryer interface {
	Do(ctx context.Context, f AsyncRetryFunc, opts ...Option) error
}

Client code like this:

retryer,  err := asyncRetry.reserve()
if err != nil {
  // logging
} 
go func() {
      if err := retryer.Do(ctx,retryFunc)err!=nil { 
            log.Error(err.Error())
      }
}()

• Pros
  • Interface is sophisticated?
  • You can write code before and after of call of retryer.Do.
  • You may call retry.Do without launching a new goroutine. It is rare but still valid.
• Cons
  • not always safe when used in the wrong way.
    • If you reserve in a new goroutine, the same problem occur
    • If you reserve and not use retryer, shutdown never succeed
    • multiple calls of retryer.Do breaks codes. In truth, we can avoid this risk using sync.Once, but I don't want to use that because it makes performance worse
  • This is very very rare case. But maybe it is not safe if sequence of process is like this:
    • 1 retryer.Do returns error
    • 2 Go scheduler schedules, and 1's goroutine stop before logging
    • 2shutdown succeed
    • 3 the instance this program running is completely shutdown before 1's logging