bsm / redislock

Simplified distributed locking implementation using Redis
Other
1.45k stars 157 forks source link

Add watchdog to refresh the lock atomatically #73

Open WqyJh opened 11 months ago

dim commented 10 months ago

Thank you very much for the contribution, but I think there is a simpler API to achieve this. Instead of passing a Watchdog option, you could simply create a wrapper function/struct which calls Refresh at regular intervals (normally expTime * 0.4 or so).

The reason why I haven't included this into the core API is error management. I suspect everyone has slightly different approaches to error handling in background operations.

If I was to write this, I would do something like:

lock, _ := rl.Obtain(ctx, "key", 100*time.Millisecond, nil)
defer lock.Release()

refresh, _ := redislock.AutoRefresh(ctx, 40*time.Millisecond)
defer refresh.Stop()

// do what you are doing ...

// check for errors before returning
if err := refresh.Error(); err != nil {
  // handle error
  _ = err
}
dim commented 10 months ago

Would you be able to adapt your PR to support the API above? It would be far less intrusive and equally effective.

WqyJh commented 10 months ago

Thank you for your response.

Although these APIs seem promising, they necessitate a substantial increase in the number of lines of code due to the requirement to handle errors. This can result in repetitive lock and refresh logic if there are multiple locks in the source code.

In my view, Auto-Refresh is a beneficial feature that should be user-friendly and require minimal coding. Even the current solution of adding a watchdog is too complex. I believe it would be more advantageous to integrate it as a higher-level configuration within the *redislock.Client, rather than adding more APIs or adding configuration to existing ones.

WqyJh commented 10 months ago

Thank you very much for the contribution, but I think there is a simpler API to achieve this. Instead of passing a Watchdog option, you could simply create a wrapper function/struct which calls Refresh at regular intervals (normally expTime * 0.4 or so).

The reason why I haven't included this into the core API is error management. I suspect everyone has slightly different approaches to error handling in background operations.

If I was to write this, I would do something like:

lock, _ := rl.Obtain(ctx, "key", 100*time.Millisecond, nil)
defer lock.Release()

refresh, _ := redislock.AutoRefresh(ctx, 40*time.Millisecond)
defer refresh.Stop()

// do what you are doing ...

// check for errors before returning
if err := refresh.Error(); err != nil {
  // handle error
  _ = err
}

I have implemented the following code of AutoRefresh, which will only return on ErrNotObtained or a context error. Other errors will be ignored. However, I am not sure if refresh.Error() is necessary. If it is, then errors should not be ignored and the first error should be returned. Do you have any suggestions?

func (l *Lock) AutoRefresh(ctx context.Context, interval time.Duration) (stop func(), err error) {
    ch := make(chan struct{})
    var cancel context.CancelFunc
    ctx, cancel = context.WithCancel(ctx)

    go func() {
        ticker := time.NewTicker(interval)
        defer ticker.Stop()
        defer func() {
            cancel()
            close(ch)
        }()

        for {
            select {
            case <-ctx.Done():
                return
            case <-ticker.C:
                err := l.Refresh(ctx, l.ttl, nil)
                if err != nil {
                    if err == ErrNotObtained {
                        return
                    }
                    // continue on other errors
                }
            }
        }
    }()

    return func() {
        cancel()
        <-ch
    }, nil
}