Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mock: update callString to format context.Context as pointer #1689

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

mock: update callString to format context.Context as pointer #1689

wants to merge 1 commit into from

Conversation

jmank88
Copy link

@jmank88 jmank88 commented Dec 18, 2024 •
edited
Loading

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/toolchain@v0.0.1-go1.23.4.linux-amd64/src/runtime/race_amd64.s:281 +0xb
  sync/atomic.AddInt32()
      <autogenerated>:1 +0x14
  context.(*cancelCtx).cancel()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/src/context/context.go:561 +0x2e7
  context.WithCancel.func1()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/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/toolchain@v0.0.1-go1.23.4.linux-amd64/src/reflect/value.go:1458 +0x544
  fmt.(*pp).printValue()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/src/fmt/print.go:792 +0x4a0
  fmt.(*pp).printValue()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/src/fmt/print.go:853 +0x1d1e
  fmt.(*pp).printValue()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/src/fmt/print.go:853 +0x1d1e
  fmt.(*pp).printValue()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/src/fmt/print.go:921 +0x130a
  fmt.(*pp).printArg()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/src/fmt/print.go:759 +0xb84
  fmt.(*pp).doPrintf()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/src/fmt/print.go:1074 +0x5cf
  fmt.Sprintf()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/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/toolchain@v0.0.1-go1.23.4.linux-amd64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/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/toolchain@v0.0.1-go1.23.4.linux-amd64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/src/testing/testing.go:1743 +0x44

Goroutine 9 (running) created at:
  testing.(*T).Run()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/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/toolchain@v0.0.1-go1.23.4.linux-amd64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /home/jordan/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.linux-amd64/src/testing/testing.go:1743 +0x44
==================

@jmank88 jmank88 marked this pull request as ready for review December 19, 2024 18:47
@brackendawson
Copy link
Collaborator

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")?

@jmank88
Copy link
Author

jmank88 commented Mar 24, 2025

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/testify@v1.10.0/mock/mock.go:453 +0x4ae
  github.com/stretchr/testify/mock.(*Mock).MethodCalled()
      /home/runner/go/pkg/mod/github.com/stretchr/testify@v1.10.0/mock/mock.go:517 +0x78d
  github.com/stretchr/testify/mock.(*Mock).Called()
      /home/runner/go/pkg/mod/github.com/stretchr/testify@v1.10.0/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/evm@v0.0.0-20250317165050-f8c18e1f415e/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/chains@v0.0.0-20250317154237-afa0f9b81806/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/chains@v0.0.0-20250317154237-afa0f9b81806/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/chainlink-common@v0.5.1-0.20250320205517-3c9893acc471/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/chains@v0.0.0-20250317154237-afa0f9b81806/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/chains@v0.0.0-20250317154237-afa0f9b81806/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/chainlink-common@v0.5.1-0.20250320205517-3c9893acc471/pkg/services/stop.go:55 +0x111
  github.com/smartcontractkit/chainlink-common/pkg/services.StopRChan.Ctx()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-common@v0.5.1-0.20250320205517-3c9893acc471/pkg/services/stop.go:44 +0x48
  github.com/smartcontractkit/chainlink-common/pkg/services.StopRChan.NewCtx()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-common@v0.5.1-0.20250320205517-3c9893acc471/pkg/services/stop.go:39 +0x34
  github.com/smartcontractkit/chainlink-common/pkg/services.StopChan.NewCtx()
      /home/runner/go/pkg/mod/github.com/smartcontractkit/chainlink-common@v0.5.1-0.20250320205517-3c9893acc471/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/chains@v0.0.0-20250317154237-afa0f9b81806/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/chains@v0.0.0-20250317154237-afa0f9b81806/txmgr/broadcaster.go:217 +0x84
==================

@brackendawson
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jmank88 @brackendawson