[FEATURE] RunWithLock (wrapper) (#1) #6

im-kulikov · 2017-11-10T18:54:01Z

type name will be used as lock.LockOptions by other packages, and that stutters; consider calling this Options (golint)
feature RunWithLock
add go 1.9.2
coveralls
improve caching

* type name will be used as lock.LockOptions by other packages, and that stutters; consider calling this Options (golint) * feature RunWithLock * add go 1.9.2 * coveralls * 🤦 i must read before insert * fix from http://docs.coveralls.io/go * we must do it once * improve caching

im-kulikov · 2017-11-10T19:12:11Z

fail when run coveralls.. https://travis-ci.org/im-kulikov/redis-lock/builds/300300629?utm_source=github_status&utm_medium=notification

* fix linter `collides with package name` * disable coveralls (not work in PR)

cristaloleg · 2017-11-12T17:23:03Z

.travis.yml

 go:
  - 1.6.4
  - 1.7.5
  - 1.8
+  - 1.9.2


Can be simplified to 1.9.x for 1.9 branch(1.8.x etc)

cristaloleg · 2017-11-12T17:23:22Z

lock.go

+
+		if l.opts.RetriesCount > 0 && retries <= 0 {
+			break
+		} else if l.opts.RetriesCount > 0 {


else is not needed ;)

cristaloleg · 2017-11-12T17:24:06Z

lock_test.go

@@ -197,6 +218,93 @@ var _ = Describe("Lock", func() {
 		Expect(res).To(Equal(int32(1)))
 	})

+	It("should run with locks and prevent fuzzing", func() {
+		res := int32(0)
+		wg := new(sync.WaitGroup)


AFAIK var wg sync.WaitGroup is more preferred style.

cristaloleg · 2017-11-12T17:25:41Z

lock_test.go

+					if err != nil {
+						atomic.AddInt32(&res, 100)
+						return
+					} else if !ok {


else is not needed or ok, err := lock.Lock() can be moved to if-statement.

cristaloleg · 2017-11-12T17:26:03Z

lock_test.go

+				res++
+				return nil
+			}, &Options{RetriesCount: count})
+			wg.Done()


should be deferred?

nope, this will fail tests

@cristaloleg

…i/travis-build/blob/master/public/version-aliases/go.json (thanks @cristaloleg)

im-kulikov · 2017-11-12T17:56:42Z

@bsm ping 🙂

im-kulikov · 2017-11-12T19:21:58Z

@cristaloleg, @edganiukov, @DmitriyMV thanks for review

dim

Thanks for this. Before I release a new version I would like to incorporate a few additional changes. Could you please address the feedback, I will then also add a little PR on top. Thanks again!

dim · 2017-11-13T10:44:00Z

lock.go

 		return nil, err
+	} else if !ok {
+		return nil, ErrCanntGetLock


hmm, that's a breaking change, in which case I will need to release a new major version

should I roll back?

no, don't worry, just explaining that I need to bump the major version. could you please also mention this in the function comment

dim · 2017-11-13T10:46:17Z

lock.go

 }

 // NewLock creates a new distributed lock on key
-func NewLock(client RedisClient, key string, opts *LockOptions) *Lock {
+func NewLock(client RedisClient, key string, opts *Options) *Lock {


for the new major version, I would like to rename this to just New

ok, I can do this

dim · 2017-11-13T10:47:26Z

lock.go


 	token string
 	mutex sync.Mutex
 }

+// RunWithLock run some code with Redis Lock
+func RunWithLock(client RedisClient, key string, handler Handler, opts *Options) error {


could you pls change this to: func Run(client RedisClient, key string, opts *Options, handler func() error) error, i.e. rename func name and put the handler at the end?

ok, no problems

dim · 2017-11-13T10:47:44Z

lock.go

 // RedisClient is a minimal client interface
 type RedisClient interface {
 	SetNX(key string, value interface{}, expiration time.Duration) *redis.BoolCmd
 	Eval(script string, keys []string, args ...interface{}) *redis.Cmd
 }

+// Handler is method wrapper
+type Handler func() error


I think there is no need for this, handler can be just inlined

dim · 2017-11-13T10:48:26Z

lock.go

@@ -13,35 +14,52 @@ import (
 const luaRefresh = `if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("pexpire", KEYS[1], ARGV[2]) else return 0 end`
 const luaRelease = `if redis.call("get", KEYS[1]) == ARGV[1] then return redis.call("del", KEYS[1]) else return 0 end`

+var ErrCanntGetLock = errors.New("can't get lock")


could you pls rename this to ErrCannotGetLock = errors.New("cannot get lock")

hm.. no problems

dim · 2017-11-13T10:49:01Z

lock.go

 	if opts == nil {
-		opts = new(LockOptions)
+		opts = new(Options)
 	}
 	return &Lock{client: client, key: key, opts: *opts.normalize()}


since you are at it, maybe rename the struct from Lock to Locker too?

im-kulikov · 2017-11-13T11:02:53Z

@dim yep.. im@kulikov.im

im-kulikov · 2017-11-13T11:14:30Z

@dim oops, last commit was not included in the merge, can you remerge?

im-kulikov · 2017-11-13T11:16:24Z

or rebase master with this changes

dim · 2017-11-13T11:58:40Z

@im-kulikov don't worry, I will fix in my PR. Just one question: how is RetriesCount useful? I am struggling a bit with it as it makes things much more complicated for no obvious reason. You already have WaitRetry which is a delay between retries and a WaitTimeout which is the overall retry timeout. Given these two values, (additionally) counting the attempts seems unnecessary, no?

im-kulikov · 2017-11-13T12:06:40Z

@dim hmm.. not quite. look, if you want to repeat only a certain number of times - it is enough to specify RetriesCount will happen only this number of attempts with a delay in WaitRetry

WaitTimeout suggests that you yourself need to count the number of attempts and specify it in the form of time.Duration

dim · 2017-11-13T12:11:08Z

I think that's the bit I don't "get", I don't quite see a use case for:

if you want to repeat only a certain number of times

As a user I may want to limit the time I am willing to wait to get a lock, but do I really care how many calls are made to redis in this time? (although you can even kind-of control that via WaitRetry).

Sorry, I really feel this is making the API unnecessarily complicated. At the moment, to enable retries, you only need to set WaitTimeout > 0 but with your patch, you also need to enable RetriesCount which is not really intuitive in my opinion

dim · 2017-11-13T12:17:03Z

@im-kulikov pls take a look at #7

im-kulikov · 2017-11-13T12:17:11Z

@dim In fact, nothing is changed. Only added option to specify the number of attempts.
That is, if I want to use the WaitTimeout and WaitRetry - I is sufficient to specify a WaitTimeout (and, alternatively, to change WaitRetry).
If I want to make a few attempts - to me it is enough to specify RetriesCount

dim · 2017-11-13T12:22:02Z

Sorry, but can you please explain the use case for this? As said ...

I really feel this is making the API unnecessarily complicated. At the moment, to enable retries, you only need to set WaitTimeout > 0 but with your patch, you also need to enable RetriesCount which is not really intuitive in my opinion

im-kulikov · 2017-11-13T12:25:26Z

opts = Options{RetriesCount: 3}
RunWithLock(client, key, &opts, func() error {
  //... do something
  return nil
})

will be made 3 attempts ≈ 30 ms (WaitRetry(10ms) * RetriesCount(3) = 30ms)

dim · 2017-11-13T12:28:08Z

No, I understand that, but if we go down this route, I would consider removing WaitTimeout. I don't think it makes sense to have both. Will update #7

* remove duplicate code (thanks @edganiukov)

740f425

* fix linter `collides with package name` * disable coveralls (not work in PR)

cristaloleg reviewed Nov 12, 2017

View reviewed changes

im-kulikov added 3 commits November 12, 2017 20:36

* review comments fixes (thanks @cristaloleg)

132b122

* fix linter-warning: lock collides with package name

69d9c9c

* fix travis for latest stable versions - https://github.com/travis-c…

59606e0

…i/travis-build/blob/master/public/version-aliases/go.json (thanks @cristaloleg)

* defer wg.Done + fix test (thanks @edganiukov and @DmitriyMV)

2662b10

dim reviewed Nov 13, 2017

View reviewed changes

im-kulikov added 2 commits November 13, 2017 14:09

* review comments fixes (thanks @dim)

386b4ba

* ObtainLock added comment for clarification (thanks @dim)

f8b8961

dim merged commit cf92a2f into bsm:master Nov 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] RunWithLock (wrapper) (#1) #6

[FEATURE] RunWithLock (wrapper) (#1) #6

im-kulikov commented Nov 10, 2017

im-kulikov commented Nov 10, 2017

cristaloleg Nov 12, 2017

cristaloleg Nov 12, 2017

cristaloleg Nov 12, 2017

cristaloleg Nov 12, 2017

cristaloleg Nov 12, 2017

im-kulikov Nov 12, 2017

im-kulikov commented Nov 12, 2017

im-kulikov commented Nov 12, 2017

dim left a comment

dim Nov 13, 2017

im-kulikov Nov 13, 2017

dim Nov 13, 2017

dim Nov 13, 2017

im-kulikov Nov 13, 2017

dim Nov 13, 2017

im-kulikov Nov 13, 2017

dim Nov 13, 2017

dim Nov 13, 2017

im-kulikov Nov 13, 2017

dim Nov 13, 2017

im-kulikov commented Nov 13, 2017

im-kulikov commented Nov 13, 2017 •

edited

Loading

im-kulikov commented Nov 13, 2017

dim commented Nov 13, 2017

im-kulikov commented Nov 13, 2017 •

edited

Loading

dim commented Nov 13, 2017

dim commented Nov 13, 2017

im-kulikov commented Nov 13, 2017

dim commented Nov 13, 2017

im-kulikov commented Nov 13, 2017 •

edited

Loading

dim commented Nov 13, 2017

[FEATURE] RunWithLock (wrapper) (#1) #6

[FEATURE] RunWithLock (wrapper) (#1) #6

Conversation

im-kulikov commented Nov 10, 2017

im-kulikov commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-kulikov commented Nov 12, 2017

im-kulikov commented Nov 12, 2017

dim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-kulikov commented Nov 13, 2017

im-kulikov commented Nov 13, 2017 • edited Loading

im-kulikov commented Nov 13, 2017

dim commented Nov 13, 2017

im-kulikov commented Nov 13, 2017 • edited Loading

dim commented Nov 13, 2017

dim commented Nov 13, 2017

im-kulikov commented Nov 13, 2017

dim commented Nov 13, 2017

im-kulikov commented Nov 13, 2017 • edited Loading

dim commented Nov 13, 2017

im-kulikov commented Nov 13, 2017 •

edited

Loading

im-kulikov commented Nov 13, 2017 •

edited

Loading

im-kulikov commented Nov 13, 2017 •

edited

Loading