This repository was archived by the owner on Aug 2, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 109
pss: Refactor. Step 2. Refactor forward cache #1742
Merged
nolash
merged 8 commits into
ethersphere:master
from
epiclabs-io:pss-refactor-forwardCache
Sep 18, 2019
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
029b8c9
pss: refactor forward_cache. Started extracting into own package
jpeletier 251ae39
pss: Refactored forwardCache into a TTLSet
jpeletier 24a2147
Added tilina/clock mockable time package for testing
jpeletier 8c7a8e5
pss/ttlset: carved out ticker for testability
jpeletier fed32b6
pss: Added metrics back
jpeletier 77c7a74
pss: take ticker out of ttlset
jpeletier b0232cc
pss: removed ut
jpeletier 5205a71
pss: vendor in tilina/clock
jpeletier File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package ticker | ||
|
|
||
| import ( | ||
| "errors" | ||
| "time" | ||
|
|
||
| "github.com/tilinna/clock" | ||
| ) | ||
|
|
||
| // Config defines the necessary information and dependencies to instantiate a Ticker | ||
| type Config struct { | ||
| Clock clock.Clock | ||
| Interval time.Duration | ||
| Callback func() | ||
| } | ||
|
|
||
| // Ticker represents a periodic timer that invokes a callback | ||
| type Ticker struct { | ||
| quitC chan struct{} | ||
| } | ||
|
|
||
| // ErrAlreadyStopped is returned if this service was already stopped and Stop() is called again | ||
| var ErrAlreadyStopped = errors.New("Already stopped") | ||
|
|
||
| // New builds a ticker that will call the given callback function periodically | ||
| func New(config *Config) *Ticker { | ||
|
|
||
| tk := &Ticker{ | ||
| quitC: make(chan struct{}), | ||
| } | ||
| ticker := config.Clock.NewTicker(config.Interval) | ||
| go func() { | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| config.Callback() | ||
| case <-tk.quitC: | ||
| return | ||
| } | ||
| } | ||
| }() | ||
| return tk | ||
| } | ||
|
|
||
| // Stop stops the timer and releases the goroutine running it. | ||
| func (tk *Ticker) Stop() error { | ||
| if tk.quitC == nil { | ||
| return ErrAlreadyStopped | ||
| } | ||
| close(tk.quitC) | ||
| tk.quitC = nil | ||
| return nil | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| package ticker_test | ||
|
|
||
| import ( | ||
| "sync" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/ethersphere/swarm/pss/internal/ticker" | ||
| "github.com/tilinna/clock" | ||
| ) | ||
|
|
||
| // TestNewTicker tests whether the ticker calls a callback function periodically | ||
| func TestNewTicker(t *testing.T) { | ||
| var err error | ||
|
|
||
| testClock := clock.NewMock(time.Unix(0, 0)) | ||
| interval := 10 * time.Second | ||
|
|
||
| wg := sync.WaitGroup{} | ||
| wg.Add(10) | ||
| tickWait := make(chan bool) | ||
|
|
||
| testTicker := ticker.New(&ticker.Config{ | ||
| Interval: interval, | ||
| Clock: testClock, | ||
| Callback: func() { | ||
| wg.Done() | ||
| tickWait <- true | ||
| }, | ||
| }) | ||
|
|
||
| for i := 0; i < 10; i++ { | ||
| testClock.Add(interval) | ||
| <-tickWait | ||
| } | ||
|
|
||
| wg.Wait() | ||
| err = testTicker.Stop() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| err = testTicker.Stop() | ||
| if err != ticker.ErrAlreadyStopped { | ||
| t.Fatal("Expected Stop() to return ticker.ErrAlreadyStopped when trying to stop an already stopped ticker") | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| package ttlset | ||
|
|
||
| import ( | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/tilinna/clock" | ||
| ) | ||
|
|
||
| // Config defines the TTLSet configuration | ||
| type Config struct { | ||
| EntryTTL time.Duration // time after which items are removed | ||
| Clock clock.Clock // time reference | ||
| } | ||
|
|
||
| // TTLSet implements a Set that automatically removes expired keys | ||
| // after a predefined expiration time | ||
| type TTLSet struct { | ||
| Config | ||
| set map[interface{}]setEntry | ||
| lock sync.RWMutex | ||
| } | ||
|
|
||
| type setEntry struct { | ||
| expiresAt time.Time | ||
| } | ||
|
|
||
| // New instances a TTLSet | ||
| func New(config *Config) *TTLSet { | ||
| ts := &TTLSet{ | ||
| set: make(map[interface{}]setEntry), | ||
| Config: *config, | ||
| } | ||
| return ts | ||
| } | ||
|
|
||
| // Add adds a new key to the set | ||
| func (ts *TTLSet) Add(key interface{}) error { | ||
| var entry setEntry | ||
| var ok bool | ||
|
|
||
| ts.lock.Lock() | ||
| defer ts.lock.Unlock() | ||
|
|
||
| if entry, ok = ts.set[key]; !ok { | ||
| entry = setEntry{} | ||
| } | ||
| entry.expiresAt = ts.Clock.Now().Add(ts.EntryTTL) | ||
| ts.set[key] = entry | ||
| return nil | ||
| } | ||
|
|
||
| // Has returns whether or not a key is already/still in the set | ||
| func (ts *TTLSet) Has(key interface{}) bool { | ||
| ts.lock.Lock() | ||
| defer ts.lock.Unlock() | ||
|
|
||
| entry, ok := ts.set[key] | ||
| if ok { | ||
| if entry.expiresAt.After(ts.Clock.Now()) { | ||
| return true | ||
| } | ||
| delete(ts.set, key) // since we're holding the lock, take the chance to delete a expired record | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // GC will remove expired entries from the set | ||
| func (ts *TTLSet) GC() { | ||
| ts.lock.Lock() | ||
| defer ts.lock.Unlock() | ||
| for k, v := range ts.set { | ||
| if v.expiresAt.Before(ts.Clock.Now()) { | ||
| delete(ts.set, k) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Count returns the number of entries in the set | ||
| func (ts *TTLSet) Count() int { | ||
| ts.lock.Lock() | ||
| defer ts.lock.Unlock() | ||
| return len(ts.set) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,150 @@ | ||
| package ttlset_test | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/ethersphere/swarm/pss/internal/ttlset" | ||
| "github.com/tilinna/clock" | ||
| ) | ||
|
|
||
| func TestTTLSet(t *testing.T) { | ||
| var err error | ||
|
|
||
| testClock := clock.NewMock(time.Unix(0, 0)) | ||
|
|
||
| testEntryTTL := 10 * time.Second | ||
| testSet := ttlset.New(&ttlset.Config{ | ||
| EntryTTL: testEntryTTL, | ||
| Clock: testClock, | ||
| }) | ||
|
|
||
| key1 := "some key" | ||
| key2 := "some other key" | ||
|
|
||
| // check adding a key to the set | ||
| err = testSet.Add(key1) | ||
| if err != nil { | ||
| t.Fatal((err)) | ||
| } | ||
|
|
||
| // check if the key is now there: | ||
| hasKey := testSet.Has(key1) | ||
| if !(hasKey == true) { | ||
| t.Fatal("key1 should've been in the set, but Has() returned false") | ||
| } | ||
|
|
||
| // check if Has() returns false when asked about a key that was never added: | ||
| hasKey = testSet.Has("some made up key") | ||
| if !(hasKey == false) { | ||
| t.Fatal("Has() should have returned false when presented with a key that was never added") | ||
| } | ||
|
|
||
| // Let some time pass, but not enough to have the key expire: | ||
| testClock.Add(testEntryTTL / 2) | ||
|
|
||
| // check if the key is still there: | ||
| hasKey = testSet.Has(key1) | ||
| if !(hasKey == true) { | ||
| t.Fatal("key1 should've been in the set, but Has() returned false") | ||
| } | ||
|
|
||
| // Let some time pass well beyond the expiry time, so key1 expires: | ||
| testClock.Add(testEntryTTL * 2) | ||
|
|
||
| // Add another key to the set: | ||
| err = testSet.Add(key2) | ||
| if err != nil { | ||
| t.Fatal((err)) | ||
| } | ||
|
|
||
| hasKey = testSet.Has(key1) | ||
| if !(hasKey == false) { | ||
| t.Fatal("key1 should've been removed from the set, but Has() returned true") | ||
| } | ||
|
|
||
| hasKey = testSet.Has(key2) | ||
| if !(hasKey == true) { | ||
| t.Fatal("key should remain in the set, but Has() returned false") | ||
| } | ||
|
|
||
| // Let some time pass well beyond key2's expiry time, so key2 expires: | ||
| testClock.Add(testEntryTTL * 2) | ||
|
|
||
| hasKey = testSet.Has(key2) | ||
| if !(hasKey == false) { | ||
| t.Fatal("key2 should have been wiped, but Has() returned true") | ||
| } | ||
| } | ||
|
|
||
| func TestGC(t *testing.T) { | ||
| var err error | ||
|
|
||
| testClock := clock.NewMock(time.Unix(0, 0)) | ||
|
|
||
| testEntryTTL := 10 * time.Second | ||
| testSet := ttlset.New(&ttlset.Config{ | ||
| EntryTTL: testEntryTTL, | ||
| Clock: testClock, | ||
| }) | ||
|
|
||
| key1 := "some key" | ||
| key2 := "some later key" | ||
|
|
||
| // check adding a message to the cache | ||
| err = testSet.Add(key1) | ||
| if err != nil { | ||
| t.Fatal((err)) | ||
| } | ||
|
|
||
| // move the clock 2 seconds | ||
| testClock.Add(2 * time.Second) | ||
|
|
||
| // add a second key which will have a later expiration time | ||
| err = testSet.Add(key2) | ||
| if err != nil { | ||
| t.Fatal((err)) | ||
| } | ||
|
|
||
| count := testSet.Count() | ||
| if !(count == 2) { | ||
| t.Fatal("Expected the set to contain 2 keys") | ||
| } | ||
|
|
||
| testSet.GC() // attempt a cleanup. This cleanup should not affect any of the two keys, since they are not expired. | ||
|
|
||
| count = testSet.Count() | ||
| if !(count == 2) { | ||
| t.Fatal("Expected the set to still contain 2 keys") | ||
| } | ||
|
|
||
| //Now, move the clock forward 9 seconds. This will expire key1 but still keep key2 | ||
| testClock.Add(9 * time.Second) | ||
| testSet.GC() // invoke the internal cleaning function, which should wipe only key1 | ||
| count = testSet.Count() | ||
| if !(count == 1) { | ||
| t.Fatal("Expected the set to now have only 1 key") | ||
| } | ||
|
|
||
| //Verify if key1 was wiped but key2 persists: | ||
| hasKey := testSet.Has(key1) | ||
| if !(hasKey == false) { | ||
| t.Fatal("Expected the set to have removed key1") | ||
| } | ||
|
|
||
| hasKey = testSet.Has(key2) | ||
| if !(hasKey == true) { | ||
| t.Fatal("Expected the set to still contain key2") | ||
| } | ||
|
|
||
| //Now, move the clock some more time. This will wipe key2 | ||
| testClock.Add(7 * time.Second) | ||
| testSet.GC() // invoke the internal cleaning function, which should wipe only key1 | ||
|
|
||
| count = testSet.Count() | ||
| // verify the map is now empty | ||
| if !(count == 0) { | ||
| t.Fatal("Expected the set to be empty") | ||
| } | ||
|
|
||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing that the reason for including a new dependency here is quick time travel to replace the original sleeps.
In this case it is being used for a rather simple test. Do you really feel it's warranted to invite external code in just for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to introduce the practice of injecting the clock as a dependency for each component that requires a time reference throughout Swarm. I missed this in Feeds.
This
tilina/clockpackage takes care of offering both a mock clock for tests and a passthrough one for runtime, both implementing the same interface, which is identical totime, so people know what to expect. The lib is small and well tested.Code that uses
time.Now()directly cannot be tested reliably. A component should use the injected time reference.As a result, all these tests run predictably and instantaneously.
If this is approved, I'd like to refactor Feeds to use it as well.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the advantage of it. It's fine by me, and I trust you when you vouch for it.
But this might end up being a discussion of course, so when you say:
maybe for the record say something about what that means? Well tested by you? Other endorsements? Even though it's small we should still be mindful.
To be honest I don't know the protocol for adding new deps to Swarm. I am sure for example @janos does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well tested means I read the tests and are sufficient. I would have written it myself following the same pattern and include it in Swarm if necessary.
The last thing I want is to start a discussion. If this is approach is not accepted I will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has a new dependency added nicely. There is no problem with that. And for the clock dependency itself, I think that it has very nice api and that it is useful. Go lack of mocking time is very annoying. We can argue if another similar package can be used, given the popularity and development activity for this one compared to others. But I think that it is ok.