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

[FAB-18329] Fix data race in cluster/comm_test#TestRenewCertificates #2089

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

yacovm
Copy link
Contributor

@yacovm yacovm commented Nov 9, 2020

This commit fixes a data race in orderer/common/cluster/comm_test.go#TestRenewCertificates.

The race occurred because the TLS configuration was updated from the test goroutine,
while a gRPC connection was being established from another goroutine, and as a result,
the TLS configuration was loaded without memory synchronization with the first goroutine.

I made the test close the gRPC connections before reconfiguring the communication layer.

Change-Id: I82b9cc685e9160e480ce77ec7e0a233b106eb0e5
Signed-off-by: Yacov Manevich yacovm@il.ibm.com

@yacovm yacovm requested a review from a team as a code owner November 9, 2020 19:16
caod123
caod123 previously approved these changes Nov 9, 2020
Copy link

@caod123 caod123 left a comment

Choose a reason for hiding this comment

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

Verified with go test -run TestRenewCertificate --race -count=100 -failfast

caod123
caod123 previously approved these changes Nov 9, 2020
This commit fixes a data race in orderer/common/cluster/comm_test.go#TestRenewCertificates.

The race occurred because the TLS configuration was updated from the test goroutine,
while a gRPC connection was being established from another goroutine, and as a result,
the TLS configuration was loaded without memory synchronization with the first goroutine.

I made the test close the gRPC connections before reconfiguring the communication layer.

Change-Id: I82b9cc685e9160e480ce77ec7e0a233b106eb0e5
Signed-off-by: Yacov Manevich <yacovm@il.ibm.com>
@yacovm
Copy link
Contributor Author

yacovm commented Nov 9, 2020

Verified with go test -run TestRenewCertificate --race -count=100 -failfast

Yet it failed the first time it ever ran in CI.
These VMs are jinxed. No other explanation.

@caod123 caod123 merged commit 901fe6c into hyperledger:master Nov 9, 2020
@yacovm yacovm deleted the FAB-18329 branch November 9, 2020 22:59
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