testify icon indicating copy to clipboard operation
testify copied to clipboard

Published 20 hours ago • verified publisher icon stretchr

mock: update callString to format context.Context as pointer

Open jmank88 opened this issue 11 months ago • 3 comments

Summary

This PR proposes updating func callString in package mock to format context.Context arguments as their pointer value, in order to avoid a data race from formatting the internals as a string.

Changes

  • changed func callString to format context.Context args as their pointer value
  • added subtest Test_callString/context to validate

Motivation

This is an example race detected from the new test Test_callString/context, before including the fix:

Data Race 
==================
WARNING: DATA RACE
Write at 0x00c000016470 by goroutine 10:
  sync/atomic.AddInt32()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/runtime/race_amd64.s:281 +0xb
  sync/atomic.AddInt32()
      <autogenerated>:1 +0x14
  context.(*cancelCtx).cancel()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/context/context.go:561 +0x2e7
  context.WithCancel.func1()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/context/context.go:237 +0x52
  github.com/stretchr/testify/mock.Test_callString.func1.2()
      /home/jordan/testify/mock/mock_test.go:1248 +0x99

Previous read at 0x00c000016470 by goroutine 9:
  reflect.Value.Int()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/reflect/value.go:1458 +0x544
  fmt.(*pp).printValue()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/fmt/print.go:792 +0x4a0
  fmt.(*pp).printValue()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/fmt/print.go:853 +0x1d1e
  fmt.(*pp).printValue()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/fmt/print.go:853 +0x1d1e
  fmt.(*pp).printValue()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/fmt/print.go:921 +0x130a
  fmt.(*pp).printArg()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/fmt/print.go:759 +0xb84
  fmt.(*pp).doPrintf()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/fmt/print.go:1074 +0x5cf
  fmt.Sprintf()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/fmt/print.go:239 +0x5c
  github.com/stretchr/testify/mock.callString()
      /home/jordan/testify/mock/mock.go:453 +0x4ae
  github.com/stretchr/testify/mock.Test_callString.func1()
      /home/jordan/testify/mock/mock_test.go:1253 +0x217
  testing.tRunner()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1743 +0x44

Goroutine 10 (running) created at:
  github.com/stretchr/testify/mock.Test_callString.func1()
      /home/jordan/testify/mock/mock_test.go:1245 +0x19c
  testing.tRunner()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1743 +0x44

Goroutine 9 (running) created at:
  testing.(*T).Run()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1743 +0x825
  github.com/stretchr/testify/mock.Test_callString()
      /home/jordan/testify/mock/mock_test.go:1238 +0x195
  testing.tRunner()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /home/jordan/go/pkg/mod/golang.org/[email protected]/src/testing/testing.go:1743 +0x44
==================

jmank88 avatar Dec 18 '24 18:12 jmank88

Similar to but not the same as #1597.

I'm not a fan of listing types that are more prone to containing values used on different goroutines, feels like whack-a-mole.

A compilable example would be helpful here. I'm wondering things like; was the Context actually being matched or did it use mock.Anything/mock.AnythingOfType("context.Context")?

brackendawson avatar Mar 23 '25 13:03 brackendawson

I would also prefer a solution that isn't specific to context.Context, but in practice for us this problem happens repeatedly with context.Context and I have never seen it occur with another type.

A compilable example would be helpful here. I'm wondering things like; was the Context actually being matched or did it use mock.Anything/mock.AnythingOfType("context.Context")?

Typically mock.Anything, but what is the relevance? The problem occurs during printing of arguments. Would that be altered by using AnythingOfType?

Here is a more complete race: 
==================
WARNING: DATA RACE
Read at 0x00c001457648 by goroutine 660918:
  reflect.Value.IsNil()
      /opt/hostedtoolcache/go/1.24.0/x64/src/reflect/value.go:1556 +0x94
  fmt.getField()
      /opt/hostedtoolcache/go/1.24.0/x64/src/fmt/print.go:343 +0x1c
  fmt.(*pp).printValue()
      /opt/hostedtoolcache/go/1.24.0/x64/src/fmt/print.go:853 +0x1ce4
  fmt.(*pp).printValue()
      /opt/hostedtoolcache/go/1.24.0/x64/src/fmt/print.go:853 +0x1d0c
  fmt.(*pp).printValue()
      /opt/hostedtoolcache/go/1.24.0/x64/src/fmt/print.go:921 +0x1304
  fmt.(*pp).printArg()
      /opt/hostedtoolcache/go/1.24.0/x64/src/fmt/print.go:759 +0xb64
  fmt.(*pp).doPrintf()
      /opt/hostedtoolcache/go/1.24.0/x64/src/fmt/print.go:1074 +0x5bc
  fmt.Sprintf()
      /opt/hostedtoolcache/go/1.24.0/x64/src/fmt/print.go:239 +0x5c
  github.com/stretchr/testify/mock.callString()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/mock/mock.go:453 +0x4ae
  github.com/stretchr/testify/mock.(*Mock).MethodCalled()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/mock/mock.go:517 +0x78d
  github.com/stretchr/testify/mock.(*Mock).Called()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/mock/mock.go:481 +0x191
  github.com/smartcontractkit/chainlink-integrations/evm/client/clienttest.(*Client).PendingNonceAt()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-integrations/[email protected]/client/clienttest/client.go:1528 +0x15e
  github.com/smartcontractkit/chainlink/v2/core/chains/evm/txmgr.(*evmTxmClient).PendingNonceAt()
      /home/runner/work/chainlink/chainlink/core/chains/evm/txmgr/client.go:108 +0xa1
  github.com/smartcontractkit/chainlink/v2/core/chains/evm/txmgr.(*evmTxmClient).PendingSequenceAt()
      /home/runner/work/chainlink/chainlink/core/chains/evm/txmgr/client.go:38 +0x53
  github.com/smartcontractkit/chainlink/v2/core/chains/evm/txmgr.(*nonceTracker).SyncOnChain()
      /home/runner/work/chainlink/chainlink/core/chains/evm/txmgr/nonce_tracker.go:127 +0xbd
  github.com/smartcontractkit/chainlink/v2/core/chains/evm/txmgr.(*nonceTracker).SyncSequence()
      /home/runner/work/chainlink/chainlink/core/chains/evm/txmgr/nonce_tracker.go:112 +0x3f2
  github.com/smartcontractkit/chainlink-framework/chains/txmgr.(*Broadcaster[go.shape.*uint8,go.shape.*uint8,go.shape.[20]uint8,go.shape.[32]uint8,go.shape.[32]uint8,go.shape.int64,go.shape.struct { GasPrice *github.com/smartcontractkit/chainlink-integrations/evm/assets.Wei; github.com/smartcontractkit/chainlink-integrations/evm/gas.DynamicFee }]).monitorTxs()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-framework/[email protected]/txmgr/broadcaster.go:287 +0x47b
  github.com/smartcontractkit/chainlink-framework/chains/txmgr.(*Broadcaster[go.shape.*uint8,go.shape.*uint8,go.shape.[20]uint8,go.shape.[32]uint8,go.shape.[32]uint8,go.shape.int64,go.shape.struct { GasPrice *github.com/smartcontractkit/chainlink-integrations/evm/assets.Wei; github.com/smartcontractkit/chainlink-integrations/evm/gas.DynamicFee }]).loadAndMonitor.gowrap2()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-framework/[email protected]/txmgr/broadcaster.go:217 +0x84

Previous write at 0x00c001457648 by goroutine 660922:
  sync/atomic.StoreInt64()
      /opt/hostedtoolcache/go/1.24.0/x64/src/runtime/race_amd64.s:237 +0xb
  sync/atomic.StorePointer()
      /opt/hostedtoolcache/go/1.24.0/x64/src/runtime/atomic_pointer.go:92 +0x44
  context.(*cancelCtx).Done()
      /opt/hostedtoolcache/go/1.24.0/x64/src/context/context.go:452 +0x124
  github.com/smartcontractkit/chainlink-common/pkg/services.StopRChan.CtxCancel.func1()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/[email protected]/pkg/services/stop.go:59 +0x5a

Goroutine 660918 (running) created at:
  github.com/smartcontractkit/chainlink-framework/chains/txmgr.(*Broadcaster[go.shape.*uint8,go.shape.*uint8,go.shape.[20]uint8,go.shape.[32]uint8,go.shape.[32]uint8,go.shape.int64,go.shape.struct { GasPrice *github.com/smartcontractkit/chainlink-integrations/evm/assets.Wei; github.com/smartcontractkit/chainlink-integrations/evm/gas.DynamicFee }]).loadAndMonitor()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-framework/[email protected]/txmgr/broadcaster.go:217 +0x22e
  github.com/smartcontractkit/chainlink-framework/chains/txmgr.(*Broadcaster[go.shape.*uint8,go.shape.*uint8,go.shape.[20]uint8,go.shape.[32]uint8,go.shape.[32]uint8,go.shape.int64,go.shape.struct { GasPrice *github.com/smartcontractkit/chainlink-integrations/evm/assets.Wei; github.com/smartcontractkit/chainlink-integrations/evm/gas.DynamicFee }]).startInternal.gowrap2()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-framework/[email protected]/txmgr/broadcaster.go:202 +0x44

Goroutine 660922 (running) created at:
  github.com/smartcontractkit/chainlink-common/pkg/services.StopRChan.CtxCancel()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/[email protected]/pkg/services/stop.go:55 +0x111
  github.com/smartcontractkit/chainlink-common/pkg/services.StopRChan.Ctx()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/[email protected]/pkg/services/stop.go:44 +0x48
  github.com/smartcontractkit/chainlink-common/pkg/services.StopRChan.NewCtx()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/[email protected]/pkg/services/stop.go:39 +0x34
  github.com/smartcontractkit/chainlink-common/pkg/services.StopChan.NewCtx()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/[email protected]/pkg/services/stop.go:14 +0xe
  github.com/smartcontractkit/chainlink-framework/chains/txmgr.(*Broadcaster[go.shape.*uint8,go.shape.*uint8,go.shape.[20]uint8,go.shape.[32]uint8,go.shape.[32]uint8,go.shape.int64,go.shape.struct { GasPrice *github.com/smartcontractkit/chainlink-integrations/evm/assets.Wei; github.com/smartcontractkit/chainlink-integrations/evm/gas.DynamicFee }]).monitorTxs()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-framework/[email protected]/txmgr/broadcaster.go:282 +0xf8
  github.com/smartcontractkit/chainlink-framework/chains/txmgr.(*Broadcaster[go.shape.*uint8,go.shape.*uint8,go.shape.[20]uint8,go.shape.[32]uint8,go.shape.[32]uint8,go.shape.int64,go.shape.struct { GasPrice *github.com/smartcontractkit/chainlink-integrations/evm/assets.Wei; github.com/smartcontractkit/chainlink-integrations/evm/gas.DynamicFee }]).loadAndMonitor.gowrap2()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-framework/[email protected]/txmgr/broadcaster.go:217 +0x84
==================

jmank88 avatar Mar 24 '25 11:03 jmank88

The relevance is in considering that we might not need to format an argument matched against mock.Anything. A potential generic way of reducing the race conditions.

brackendawson avatar Mar 24 '25 14:03 brackendawson