fix: add locking around broker throttle timer to prevent race condition #2826

chengsha · 2024-03-05T02:43:33Z

The producer can be easily blocked due to a race condition in Broker.throttleTimer, which may result in a panic. Consider changing the type of Broker.throttleTimer to atomic.Value

Fixes #2823

Signed-off-by: chengsha shacheng@tencent.com

puellanivis · 2024-03-05T08:46:22Z

broker.go

 func (b *Broker) setThrottle(throttleTime time.Duration) {
-	if b.throttleTimer != nil {
+	value := b.throttleTimer.Load()
+	if value != nil && value != emptyTimer {
+		throttleTimer := value.(*time.Timer)
 		// if there is an existing timer stop/clear it
-		if !b.throttleTimer.Stop() {
-			<-b.throttleTimer.C
+		if !throttleTimer.Stop() {
+			<-throttleTimer.C
 		}
 	}
-	b.throttleTimer = time.NewTimer(throttleTime)
+	b.throttleTimer.CompareAndSwap(value, time.NewTimer(throttleTime))
 }


I’m unsure about this approach. Since this is now an atomic operation, and there is a potentially blocking operation in it, we could easily end up with two setThrottle operations running at the same time.

Of particular note is the message on the timer.Stop godoc:

This cannot be done concurrent to other receives from the Timer's channel or other calls to the Timer's Stop method.

I think we need to be handling this within a properly locked mutex.

you are right. throttleTimer.Stop is not safe there. i change to locked mutex

…throttleTimer, which may result in a panic. add throttleTimerLock to protect Fixes IBM#2823 Signed-off-by: shacheng <shacheng@tencent.com>

dnwe

Thanks for reporting and fixing this locking issue 🎉

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [github.com/IBM/sarama](https://github.com/IBM/sarama) | `v1.43.0` -> `v1.43.1` | [![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fIBM%2fsarama/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fIBM%2fsarama/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fIBM%2fsarama/v1.43.0/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fIBM%2fsarama/v1.43.0/v1.43.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>IBM/sarama (github.com/IBM/sarama)</summary> ### [`v1.43.1`](https://github.com/IBM/sarama/releases/tag/v1.43.1): Version 1.43.1 (2024-03-27) [Compare Source](https://github.com/IBM/sarama/compare/v1.43.0...v1.43.1)  #### What's Changed ##### 🐛 Fixes - fix: message.max.bytes should default to [`1048576`](https://github.com/IBM/sarama/commit/1048576) not 1 MB by [@puellanivis](https://github.com/puellanivis) in [https://github.com/IBM/sarama/pull/2804](https://github.com/IBM/sarama/pull/2804) - fix: add locking around broker throttle timer to prevent race condition by [@chengsha](https://github.com/chengsha) in [https://github.com/IBM/sarama/pull/2826](https://github.com/IBM/sarama/pull/2826) ##### 📦 Dependency updates - chore(deps): bump go.opentelemetry.io/otel/sdk from 1.23.1 to 1.24.0 in /examples/interceptors by [@dependabot](https://github.com/dependabot) in [https://github.com/IBM/sarama/pull/2816](https://github.com/IBM/sarama/pull/2816) - chore(deps): bump the golang-org-x group with 1 update by [@dependabot](https://github.com/dependabot) in [https://github.com/IBM/sarama/pull/2825](https://github.com/IBM/sarama/pull/2825) - chore(deps): bump github.com/stretchr/testify from 1.8.4 to 1.9.0 by [@dependabot](https://github.com/dependabot) in [https://github.com/IBM/sarama/pull/2822](https://github.com/IBM/sarama/pull/2822) - chore(deps): bump go.opentelemetry.io/otel/exporters/stdout/stdoutmetric from 1.23.1 to 1.24.0 in /examples/interceptors by [@dependabot](https://github.com/dependabot) in [https://github.com/IBM/sarama/pull/2815](https://github.com/IBM/sarama/pull/2815) #### New Contributors - [@chengsha](https://github.com/chengsha) made their first contribution in [https://github.com/IBM/sarama/pull/2826](https://github.com/IBM/sarama/pull/2826) **Full Changelog**: IBM/sarama@v1.43.0...v1.43.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).  --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>

This patch upgrades sarama to v1.43.1 to pick up fix IBM/sarama#2826 related to the kafka throttling feature we supported in v24.1. It addresses a race condition where producer could be blocked. Epic: none Release note: fixed a race condition in sarama when kafka throttling is enabled Epic: none Release note: fixed a race condition in sarama when kafka throttling is enabled

133563: deps: upgrade sarama@1.42.1 to version 1.43.1 r=rharding6373 a=wenyihu6 This patch upgrades sarama to v1.43.1 to pick up fix IBM/sarama#2826 related to the kafka throttling feature we supported in v24.1. It addresses a race condition where producer could be blocked. Epic: none Release note: fixed a race condition in sarama when kafka throttling is enabled Co-authored-by: Wenyi Hu <wenyi@cockroachlabs.com>

puellanivis reviewed Mar 6, 2024

View reviewed changes

chengsha force-pushed the chengsha/fix-data-race-when-throttled branch 2 times, most recently from c6d9d39 to cc3fa66 Compare March 6, 2024 01:57

The producer can be easily blocked due to a race condition in Broker.…

7d6b70b

…throttleTimer, which may result in a panic. add throttleTimerLock to protect Fixes IBM#2823 Signed-off-by: shacheng <shacheng@tencent.com>

chengsha force-pushed the chengsha/fix-data-race-when-throttled branch from cc3fa66 to 7d6b70b Compare March 7, 2024 01:55

dnwe added the fix label Mar 7, 2024

dnwe changed the title ~~The producer can be easily blocked due to a race condition in Broker.…~~ fix: add locking around broker throttle timer to prevent race condition Mar 7, 2024

dnwe approved these changes Mar 8, 2024

View reviewed changes

dnwe merged commit a8b3b3d into IBM:main Mar 8, 2024
13 checks passed

wenyihu6 mentioned this pull request Oct 28, 2024

deps: upgrade sarama@1.42.1 to version 1.43.1 cockroachdb/cockroach#133563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add locking around broker throttle timer to prevent race condition #2826

fix: add locking around broker throttle timer to prevent race condition #2826

chengsha commented Mar 5, 2024

puellanivis Mar 5, 2024

chengsha Mar 6, 2024

dnwe left a comment

fix: add locking around broker throttle timer to prevent race condition #2826

fix: add locking around broker throttle timer to prevent race condition #2826

Conversation

chengsha commented Mar 5, 2024

puellanivis Mar 5, 2024

Choose a reason for hiding this comment

chengsha Mar 6, 2024

Choose a reason for hiding this comment

dnwe left a comment

Choose a reason for hiding this comment