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

Add concurrency safeguards to the current round timeout field on chain #1342

Merged
merged 4 commits into from Jun 7, 2022

Conversation

fabioDMFerreira
Copy link
Contributor

Fixes

Add concurrency safeguards to the current round timeout field on chain (#629)
Also, add concurrency safeguards to the transaction cost as the processing of transactions may be done concurrently.

Changes

Need to be mentioned in CHANGELOG.md?

Tests

Tasks to complete before merging PR:

  • Ensure system tests are passing. If not Run them manually to check for any regressions 📋
  • Do any new system tests need added to test this change? do any existing system tests need updated? If so create a PR at 0chain/system_test
  • Merge your system tests PR to master AFTER merging this PR

Associated PRs (Link as appropriate):

  • blobber:
  • gosdk:
  • system_test:
  • zboxcli:
  • zwalletcli:
  • Other: ...

@dabasov dabasov linked an issue Jun 2, 2022 that may be closed by this pull request
@dabasov
Copy link
Member

dabasov commented Jun 2, 2022

the problem here is that tii reference escapes from generate block and most likely is used concurrently in this closure. It is set to handler and

func txnIterHandlerFunc(mc *Chain,
	b *block.Block,
	lfb *block.Block,
	bState util.MerklePatriciaTrieI,
	txnProcessor txnProcessorHandler,
	tii *TxnIterInfo) func(context.Context, datastore.CollectionEntity) bool {
	return func(ctx context.Context, qe datastore.CollectionEntity) bool {

and handler lately is passed to iterate collection where it can be used concurrently

	err := transactionEntityMetadata.GetStore().IterateCollection(cctx, transactionEntityMetadata, collectionName, txnIterHandler)

please check all tii reference sharing between go routines

@fabioDMFerreira
Copy link
Contributor Author

the problem here is that tii reference escapes from generate block and most likely is used concurrently in this closure. It is set to handler and

func txnIterHandlerFunc(mc *Chain,
	b *block.Block,
	lfb *block.Block,
	bState util.MerklePatriciaTrieI,
	txnProcessor txnProcessorHandler,
	tii *TxnIterInfo) func(context.Context, datastore.CollectionEntity) bool {
	return func(ctx context.Context, qe datastore.CollectionEntity) bool {

and handler lately is passed to iterate collection where it can be used concurrently

	err := transactionEntityMetadata.GetStore().IterateCollection(cctx, transactionEntityMetadata, collectionName, txnIterHandler)

please check all tii reference sharing between go routines

I got it. Only now, I realize tii escapes to txnProcessor which also updates fields. Thanks

@dabasov
Copy link
Member

dabasov commented Jun 6, 2022

please fix lint error

Copy link
Member

@dabasov dabasov left a comment

Choose a reason for hiding this comment

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

lgtm

@dabasov dabasov merged commit b7a2de1 into staging Jun 7, 2022
Kishan-Dhakan added a commit that referenced this pull request Jun 10, 2022
…guards-on-crt-chain-field"

This reverts commit b7a2de1, reversing
changes made to 3676668.
@peterlimg peterlimg deleted the 629-use-concurrency-safeguards-on-crt-chain-field branch December 3, 2022 02:12
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.

Possible race condition
2 participants