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

Bugfix: conditional variable broadcast channel size #3906

Merged
merged 1 commit into from
Feb 5, 2023
Merged

Bugfix: conditional variable broadcast channel size #3906

merged 1 commit into from
Feb 5, 2023

Conversation

wxing1292
Copy link
Contributor

What changed?

  • Conditional variable broadcast function should create channel with size 1, not 0

Why?
0 size channel does not guarantee ordering

when the conditional is initialized, the signal channel is correctly initialized with size 1
however (the bug) incorrectly re-initialized the signal channel with size 0
when a coroutine waits for notification, there is a time gap & lock gap between accessing the signal channel and selecting on the signal channel
so signal delivered during the above gap can be dropped

How did you test it?
Loop go test -race && UT

temporal|bugfix-broadcast ⇒ while true; do go test -timeout=24s -race -count=128 ./common/locks -run ^TestConditionVariableSuite$ -testify.m TestCase_ProducerConsumer; done
ok  	go.temporal.io/server/common/locks	16.358s
ok  	go.temporal.io/server/common/locks	16.246s
ok  	go.temporal.io/server/common/locks	16.822s
ok  	go.temporal.io/server/common/locks	16.794s
ok  	go.temporal.io/server/common/locks	16.939s
ok  	go.temporal.io/server/common/locks	16.488s
ok  	go.temporal.io/server/common/locks	16.330s
ok  	go.temporal.io/server/common/locks	16.996s
ok  	go.temporal.io/server/common/locks	16.890s
ok  	go.temporal.io/server/common/locks	16.540s
ok  	go.temporal.io/server/common/locks	16.594s
ok  	go.temporal.io/server/common/locks	16.265s
ok  	go.temporal.io/server/common/locks	16.357s
ok  	go.temporal.io/server/common/locks	16.696s
ok  	go.temporal.io/server/common/locks	17.341s
ok  	go.temporal.io/server/common/locks	16.577s
ok  	go.temporal.io/server/common/locks	16.688s
ok  	go.temporal.io/server/common/locks	17.062s
ok  	go.temporal.io/server/common/locks	16.724s
ok  	go.temporal.io/server/common/locks	17.180s
ok  	go.temporal.io/server/common/locks	16.966s
ok  	go.temporal.io/server/common/locks	17.273s
ok  	go.temporal.io/server/common/locks	17.278s
ok  	go.temporal.io/server/common/locks	17.191s
ok  	go.temporal.io/server/common/locks	17.747s
ok  	go.temporal.io/server/common/locks	17.489s
ok  	go.temporal.io/server/common/locks	17.556s
ok  	go.temporal.io/server/common/locks	17.292s
ok  	go.temporal.io/server/common/locks	16.914s
ok  	go.temporal.io/server/common/locks	16.687s
ok  	go.temporal.io/server/common/locks	16.700s
ok  	go.temporal.io/server/common/locks	16.817s
ok  	go.temporal.io/server/common/locks	17.129s
ok  	go.temporal.io/server/common/locks	16.757s
ok  	go.temporal.io/server/common/locks	17.299s
ok  	go.temporal.io/server/common/locks	17.389s
ok  	go.temporal.io/server/common/locks	17.355s
ok  	go.temporal.io/server/common/locks	17.013s
ok  	go.temporal.io/server/common/locks	16.998s
ok  	go.temporal.io/server/common/locks	17.156s
ok  	go.temporal.io/server/common/locks	16.782s

Potential risks
No risk

Is hotfix candidate?
No, broadcast function implemented but not used in production logic

* Conditional variable broadcast function should create channel with size 1, not 0
@wxing1292 wxing1292 requested a review from yycptt February 5, 2023 00:29
@wxing1292 wxing1292 requested a review from a team as a code owner February 5, 2023 00:29
@wxing1292 wxing1292 merged commit 157cd11 into temporalio:master Feb 5, 2023
@wxing1292 wxing1292 deleted the bugfix-broadcast branch February 5, 2023 03:22
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