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.

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

blockcache: fix datarace in blockcache_test #7029

Merged

Conversation

MStreet3
Copy link
Contributor

@MStreet3 MStreet3 commented Oct 13, 2022

This PR fixes an issue with a data race in the blockcache tests.

Change Description

  • use a write mutex in GetBlock instead of a read mutex.
  • use a write mutex in resetChainCallCount instead of a read mutex.
  • creates a getter method for the chainCallCount property with a read mutex.

Steps to Test

The test failure is a flake (i.e., non-deterministic), but these tests pass with the -race flag.

fixes #6915

@ellemouton ellemouton self-requested a review October 13, 2022 06:08
Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MStreet3! Thanks for the PR.

I think the only changes required to solve the linked issue are:

  1. the m.RLock() and m.RUnlock() in the GetBlock method of mockChainBackend need to converted to m.Lock() and m.Unlock()
  2. create the chainCallCount getter

blockcache/blockcache_test.go Outdated Show resolved Hide resolved
blockcache/blockcache_test.go Outdated Show resolved Hide resolved
blockcache/blockcache_test.go Outdated Show resolved Hide resolved
blockcache/blockcache_test.go Outdated Show resolved Hide resolved
@MStreet3 MStreet3 force-pushed the bug/blockcache-test-data-race branch 2 times, most recently from a5cfa02 to 6ec1db1 Compare October 13, 2022 11:56
@MStreet3 MStreet3 force-pushed the bug/blockcache-test-data-race branch 2 times, most recently from 4b076c3 to 9e22ba6 Compare October 13, 2022 12:26
Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

I think all of these changes can be squashed into one commit.
Also please add an item to the release notes for this PR

blockcache/blockcache_test.go Outdated Show resolved Hide resolved
blockcache/blockcache_test.go Show resolved Hide resolved
@MStreet3 MStreet3 force-pushed the bug/blockcache-test-data-race branch 2 times, most recently from 43ed06f to 0174c14 Compare October 13, 2022 14:28
mockChainBackend simulates a chain backend and
tracks the number of calls via a private property
called chainCallCount.  This commit uses a write
mutex instead of read mutex in the call to GetBlock,
adds a getter method to access chainCallCount
with a read mutex and uses a write mutex in
resetChainCallCount.
Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🔥

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix!

@guggero guggero merged commit 2e410dd into lightningnetwork:master Oct 13, 2022
@MStreet3 MStreet3 deleted the bug/blockcache-test-data-race branch October 13, 2022 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests: race in TestBlockCacheMutexes tests
3 participants