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

fix: make variant metrics concurrency safe #129

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

sighphyre
Copy link
Member

@sighphyre sighphyre commented Jan 24, 2023

This adds a mutex lock to the metrics for GetVariant, which fixes a crash bug which occurs when the `GetVariant`` method is called in a tight loop concurrently.

The toggle data that's modified in the countVariants is bound to the bucket, which is locked by the mutex, so I want to say this safe.

I haven't added a test because concurrency reasons but this can be arbitrarily reproduced with the following hacky code snippet from version 3.2.x to 3.7.2 (doesn't occur with equivalent IsEnabled calls).

timer := time.NewTimer(1 * time.Second)

for {
	<-timer.C

	go func() {
		for i := 0; i < 10000; i++ {
			variant := unleash.GetVariant("test")
			fmt.Printf("'%s' enabled? %v\n", "test", variant)
			timer.Reset(1 * time.Second)
		}
	}()

	go func() {
		for i := 0; i < 10000; i++ {
			variant := unleash.GetVariant("test")
			fmt.Printf("'%s' enabled? %v\n", "test", variant)
			timer.Reset(1 * time.Second)
		}
	}()
}

Comment on lines +240 to +243

m.bucketMu.Lock()
defer m.bucketMu.Unlock()

Copy link
Contributor

Choose a reason for hiding this comment

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

How does this actually work? I'm used to having the unlock after the guarded portion of the code, but maybe it's the defer what makes the difference...

@FredrikOseberg
Copy link
Contributor

FredrikOseberg commented Jan 24, 2023 via email

@sighphyre sighphyre merged commit cd3c9f4 into v3 Jan 25, 2023
@sighphyre sighphyre deleted the fix/concurrency-safe-metrics branch January 25, 2023 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants