Add limiter package #8
Conversation
Add limiter.go
Add tests for limiter.go
Add group.go
Add tests for group.go
exclude a couple of linters for limiter_test.go
Add go-deadlock
Add go-deadlock
Remove go-deadlock
Remove go-deadlock
Remove go-deadlock
Remove go-deadlock
There was a problem hiding this comment.
I'm mostly reviewing my own code here, but that's the point: we are trying to improve the developability (make it easier to read and understand) and stability (restructure to avoid the possibility of concurrency errors) of the old code, not just add tests for it.
| @@ -56,6 +56,11 @@ issues: | |||
| - path: clog.go | |||
| linters: | |||
| - gochecknoinits | |||
| - path: limiter_test.go | |||
| linters: | |||
| - nestif | |||
There was a problem hiding this comment.
Tests should themselves be clean code, so not too complicated. Extract out the complicated parts in to methods as normal. The only exception we should apply to tests is function length, since we expect lots of (simple) tests. Function length is already excluded on _test.go files.
| /******************************************************************************* | ||
| * Copyright (c) 2020 Genome Research Ltd. | ||
| * | ||
| * Author: Sendu Bala <sb10@sanger.ac.uk>. |
There was a problem hiding this comment.
The idea was to do from-scratch TDD, not copy/paste my old code and add tests afterwards...
|
|
||
| g.current-- | ||
|
|
||
| // notify callers who passed a channel to notifyDecrement(), but do it |
There was a problem hiding this comment.
Any time you have a separate block of code within a func with a comment to explain what it is, you are almost certainly looking at something that should be extracted out to its own function or struct or package.
Extract this out.
| ) | ||
|
|
||
| func TestGroup(t *testing.T) { | ||
| Convey("Test to check the group", t, func() { |
There was a problem hiding this comment.
Name tests such that the test and its nested tests form sentences that describe desired behaviour.
So in this case, it might be "Given a Group", and the first nested Convey might be "you can set the limit".
So it reads "Given a Group you can set the limit".
|
|
||
| // increment increases the current count of this group. You must call | ||
| // canIncrement() first to make sure you won't go over the limit (and hold a | ||
| // lock over the 2 calls to avoid a race condition). |
There was a problem hiding this comment.
Is there a way to restructure the whole package such that you don't have to hold an external lock across 2 methods, and instead it is able to be thread-safe internally and without room for error?
| // | ||
| // If an optional wait duration is supplied, will wait for up to the given wait | ||
| // period for an increment of every group to be possible. | ||
| func (l *Limiter) Increment(groups []string, wait ...time.Duration) bool { |
There was a problem hiding this comment.
Reimplement to take a context as first arg instead of an optional wait, and to always check for context cancellation.
| func (l *Limiter) Increment(groups []string, wait ...time.Duration) bool { | ||
| l.mu.Lock() | ||
| if l.checkGroups(groups) { | ||
| l.incrementGroups(groups) |
There was a problem hiding this comment.
This should somehow be restructured in to a single method call that does internal locking.
| l.registerGroupNotifications(groups, ch) | ||
| l.mu.Unlock() | ||
|
|
||
| continue |
| for { | ||
| select { | ||
| case <-ch: | ||
| l.mu.Lock() |
There was a problem hiding this comment.
This check, increment and then register block is the same as above. Extract in to a method regardless, and then possibly only call it from one place if you also refactor an efficient way to always wait on the context.
|
|
||
| if l.Increment(groups) { | ||
| atomic.AddUint64(&incs, 1) | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
Is there a way to do this and other tests below without apparently arbitrary sleep/wait times?
Refactor limiter.go
Add tests for group.go