+
Skip to content

Conversation

Frozen
Copy link
Collaborator

@Frozen Frozen commented Nov 9, 2023

This PR contain fixes for 4 data races i've found during testing devnet with enabled race detector. This PR arrives after we first time faced block exists error, which looks like the concurrency issue.

#4558
#4557
#4556
#4555

@Frozen Frozen marked this pull request as ready for review November 9, 2023 20:28
@Frozen Frozen self-assigned this Nov 9, 2023
@Frozen Frozen merged commit 6f7a047 into dev Nov 10, 2023
consensus.consensusTimeout[timeoutConsensus].Start()

// Send signal to Node to propose the new block for consensus
consensus.getLogger().Info().Msg("[preCommitAndPropose] sending block proposal signal")
Copy link
Collaborator

Choose a reason for hiding this comment

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

GetLogger() ?

Copy link
Collaborator Author

@Frozen Frozen Feb 17, 2024

Choose a reason for hiding this comment

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

No, we obtained lock above on line 584

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GetLogger() has lock inside, so it will be deadlock.

Copy link
Collaborator

Choose a reason for hiding this comment

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

GetLogger is a public function, it should be improved to not have such a constraints. We can't make a function public and ask users to not use it in a certain conditions, right?

// Send signal to Node to propose the new block for consensus
consensus.getLogger().Info().Msg("[preCommitAndPropose] sending block proposal signal")

consensus.mutex.Unlock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

defer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, we release lock exactly here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

to excludes line consensus.ReadySignal(AsyncProposal) ? any reason for that?

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.

2 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载