Cleanly shutdown cleanup go-routine#337
Conversation
Cleanly shutdown go routine. Big cache was made when go did not have ctx support Fixes allegro#336
| ) | ||
|
|
||
| // NewBigCache initialize new instance of BigCache | ||
| func NewBigCache(config Config) (*BigCache, error) { |
There was a problem hiding this comment.
I think we should keep the old constructor for backward compatibility but mark it deprecated
There was a problem hiding this comment.
name suggestion for the new function NewBigCacheWithContext()?
and what should we pass in the old function TODO() or Background() is fine?
There was a problem hiding this comment.
or just New() as the package name is bigcache
bigcache.New()
There was a problem hiding this comment.
Let's go with WithContext suffix as this is how std lib added context https://godocs.io/net/http#NewRequestWithContext
There was a problem hiding this comment.
Both background and todo returns the same empty context
var (
background = new(emptyCtx)
todo = new(emptyCtx)
)
There was a problem hiding this comment.
It's not about implementation, it's about readability 😉
There was a problem hiding this comment.
Saw the comment in WithContext now, went ahead with New(), looks more redable to me.
Std lib follows both, see this https://pkg.go.dev/errors#New , yes but for context addition they added WithContext
Let me know if it requires a change.
bigcache_bench_test.go
Outdated
| func BenchmarkWriteToLimitedCacheWithSmallInitSizeAnd1Shard(b *testing.B) { | ||
| m := blob('a', 1024) | ||
| cache, _ := NewBigCache(Config{ | ||
| cache, _ := NewBigCache(context.TODO(), Config{ |
There was a problem hiding this comment.
I think in tests its fine to use context.Background()
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
==========================================
- Coverage 90.17% 89.32% -0.86%
==========================================
Files 15 15
Lines 784 787 +3
==========================================
- Hits 707 703 -4
- Misses 65 71 +6
- Partials 12 13 +1
Continue to review full report at Codecov.
|
alok87
changed the title
alok87
force-pushed
the
ctx-support-cleanup-routine-fix-336
branch
from
b8dce9e to
0e0633c
Compare
alok87
force-pushed
the
ctx-support-cleanup-routine-fix-336
branch
from
0e0633c to
830f26e
Compare
alok87
force-pushed
the
ctx-support-cleanup-routine-fix-336
branch
from
830f26e to
383a05c
Compare
|
wanted to use the latest version? can we merge this? @cristaloleg @janisz |
4 participants
Cleanly shutdown go routine. Big cache was made when go did not have ctx support
Fixes #336