Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add watchdog to refresh the lock atomatically #73

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

WqyJh
Copy link

@WqyJh WqyJh commented Nov 29, 2023

No description provided.

@WqyJh WqyJh changed the title Feat watchdog Add watchdog to refresh the lock atomatically Nov 29, 2023
@dim
Copy link
Member

dim commented Dec 4, 2023

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
Copy link
Member

dim commented Dec 4, 2023

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

@WqyJh
Copy link
Author

WqyJh commented Dec 5, 2023

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
Copy link
Author

WqyJh commented Dec 29, 2023

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
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants