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
| @@ -41,11 +42,11 @@ const ( | |||
| ) | |||
|
|
|||
| // 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.
There was a problem hiding this comment.
Yeah, New is ok for me too. @janisz ?
bigcache_bench_test.go
Outdated
| @@ -16,7 +17,7 @@ func BenchmarkWriteToCacheWith1Shard(b *testing.B) { | |||
|
|
|||
| 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 |
Cleanly shutdown go routine. Big cache was made when go did not have ctx support
Fixes #336