redis / go-redis

Redis Go client
https://redis.uptrace.dev
BSD 2-Clause "Simplified" License
20.22k stars 2.38k forks source link

Data Race in context.Done() #2734

Open viv2793 opened 1 year ago

viv2793 commented 1 year ago

Issue tracker is used for reporting bugs and discussing new features. Please use stackoverflow for supporting issues.

Expected Behavior

Data race should not happen

Current Behavior

We are getting data race when we are using ctx.Done(). We are using context to pass some key:value pairs across functions. But sometimes we get this data race in case of ctx.Done()

==================
WARNING: DATA RACE
Read at 0x00c000e0c2a0 by goroutine 47:
  context.(*valueCtx).Done()
      <autogenerated>:1 +0x3c
  context.(*valueCtx).Done()
      <autogenerated>:1 +0x4e
  context.(*valueCtx).Done()
      <autogenerated>:1 +0x4e
  github.com/go-redis/redis/v8/internal/pool.(*ConnPool).waitTurn()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/internal/pool/pool.go:292 +0x61
  github.com/go-redis/redis/v8/internal/pool.(*ConnPool).Get()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/internal/pool/pool.go:249 +0x74
  github.com/go-redis/redis/v8.(*baseClient)._getConn()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/redis.go:194 +0x77
  github.com/go-redis/redis/v8.(*baseClient).getConn()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/redis.go:182 +0xc4
  github.com/go-redis/redis/v8.(*baseClient).withConn()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/redis.go:274 +0x8c
  github.com/go-redis/redis/v8.(*baseClient)._process()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/redis.go:329 +0x252
  github.com/go-redis/redis/v8.(*baseClient).process()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/redis.go:311 +0xbd
  github.com/go-redis/redis/v8.(*baseClient).process-fm()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/redis.go:306 +0x6d
  github.com/go-redis/redis/v8.hooks.process()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/redis.go:54 +0xc3
  github.com/go-redis/redis/v8.(*Client).Process()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/redis.go:596 +0xc4
  github.com/go-redis/redis/v8.(*Client).Process-fm()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/redis.go:595 +0x46
  github.com/go-redis/redis/v8.cmdable.Get()
      /go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/commands.go:786 +0x1eb
  git.egnyte-internal.com/UnifiedCMM/cmmutils/cache.CacheHandler.GetObjectFromCache()
      /go/pkg/mod/git.egnyte-internal.com/!unified!c!m!m/cmmutils@v0.3.2-0.20230712060851-6761f65611ed/cache/redis.go:33 +0x10c
  git.egnyte-internal.com/UnifiedCMM/cmmutils/utils/common.(*InMemory).GetMigration()
      /go/pkg/mod/git.egnyte-internal.com/!unified!c!m!m/cmmutils@v0.3.2-0.20230712060851-6761f65611ed/utils/common/utils.go:112 +0x17a
  git.egnyte-internal.com/UnifiedCMM/cmmutils/utils/common.GetMigrationObj()
      /go/pkg/mod/git.egnyte-internal.com/!unified!c!m!m/cmmutils@v0.3.2-0.20230712060851-6761f65611ed/utils/common/utils.go:1483 +0x90
  git.egnyte-internal.com/UnifiedCMM/cmmutils/utils/common.SetMigrationID()
      /go/pkg/mod/git.egnyte-internal.com/!unified!c!m!m/cmmutils@v0.3.2-0.20230712060851-6761f65611ed/utils/common/utils.go:961 +0xd2
  git.egnyte-internal.com/UnifiedCMM/cmmutils/pubsub.sendMessageToAgent()
      /go/pkg/mod/git.egnyte-internal.com/!unified!c!m!m/cmmutils@v0.3.2-0.20230712060851-6761f65611ed/pubsub/pubsub.go:144 +0x346
  git.egnyte-internal.com/UnifiedCMM/cmmutils/pubsub.(*GPubSubCilent).ReadMessage.func1·dwrap·1()
      /go/pkg/mod/git.egnyte-internal.com/!unified!c!m!m/cmmutils@v0.3.2-0.20230712060851-6761f65611ed/pubsub/pubsub.go:105 +0xef

Previous write at 0x00c000e0c2a0 by goroutine 197:
  context.WithValue()
      /usr/local/go/src/context/context.go:533 +0xce
  git.egnyte-internal.com/UnifiedCMM/cmmutils/utils/common.SetAgentID()
      /go/pkg/mod/git.egnyte-internal.com/!unified!c!m!m/cmmutils@v0.3.2-0.20230712060851-6761f65611ed/utils/common/utils.go:1001 +0x366
  git.egnyte-internal.com/UnifiedCMM/cmmutils/pubsub.(*GPubSubCilent).ReadMessage.func1()
      /go/pkg/mod/git.egnyte-internal.com/!unified!c!m!m/cmmutils@v0.3.2-0.20230712060851-6761f65611ed/pubsub/pubsub.go:103 +0x2f5
  cloud.google.com/go/pubsub.(*Subscription).Receive.func2.2()
      /go/pkg/mod/cloud.google.com/go/pubsub@v1.25.1/subscription.go:1184 +0xec
  cloud.google.com/go/pubsub/internal/scheduler.(*ReceiveScheduler).Add.func1()
      /go/pkg/mod/cloud.google.com/go/pubsub@v1.25.1/internal/scheduler/receive_scheduler.go:84 +0x58

Goroutine 47 (running) created at:
  git.egnyte-internal.com/UnifiedCMM/cmmutils/pubsub.(*GPubSubCilent).ReadMessage.func1()
      /go/pkg/mod/git.egnyte-internal.com/!unified!c!m!m/cmmutils@v0.3.2-0.20230712060851-6761f65611ed/pubsub/pubsub.go:105 +0x693
  cloud.google.com/go/pubsub.(*Subscription).Receive.func2.2()
      /go/pkg/mod/cloud.google.com/go/pubsub@v1.25.1/subscription.go:1184 +0xec
  cloud.google.com/go/pubsub/internal/scheduler.(*ReceiveScheduler).Add.func1()
      /go/pkg/mod/cloud.google.com/go/pubsub@v1.25.1/internal/scheduler/receive_scheduler.go:84 +0x58

Goroutine 197 (running) created at:
  cloud.google.com/go/pubsub/internal/scheduler.(*ReceiveScheduler).Add()
      /go/pkg/mod/cloud.google.com/go/pubsub@v1.25.1/internal/scheduler/receive_scheduler.go:82 +0x1dc
  cloud.google.com/go/pubsub.(*Subscription).Receive.func2()
      /go/pkg/mod/cloud.google.com/go/pubsub@v1.25.1/subscription.go:1182 +0x98d
  golang.org/x/sync/errgroup.(*Group).Go.func1()
      /go/pkg/mod/golang.org/x/sync@v0.1.0/errgroup/errgroup.go:75 +0x91

Possible Solution

Steps to Reproduce

We are not able to able to reproduce this every time. And right now, we are not sure of how we can reproduce it.

The only thing that I can share is that we are using context to pass some key:value pairs across functions. But sometimes we get this data race in case of ctx.Done()

func SetAgentID(ctx context.Context, agentID string) context.Context { return context.WithValue(ctx, log.AgentIDKey, agentID) }

jquirke commented 7 months ago

Are you sure you are not putting in corrupted context.Contexts? We can into this

By context.Context corruption, interface assignments are not atomic, so a common mistake I've seen in Uber is not assigning a new context in a goroutine from an outer scoped parent context.

Consider: https://go.dev/play/p/BpJcv5_Nu-2