redis / go-redis

Redis Go client
https://redis.uptrace.dev
BSD 2-Clause "Simplified" License
19.89k stars 2.34k forks source link

Fix Data Race in hooksMixin When Using MinIdleConns and AddHook #2814

Closed RyoMiyashita closed 9 months ago

RyoMiyashita commented 9 months ago

2730

Overview

This PR addresses a critical data race issue discovered in hooksMixin when MinIdleConns is set to a value greater than 1, and client.AddHook is used concurrently.

Problem

The data race was identified during test, specifically in scenarios where MinIdleConns triggers the creation of idle connections in a separate goroutine, while client.AddHook modifies the hooks concurrently. This concurrent access and modification of hooks.dialer led to a data race.

Solution

To resolve this concurrency issue, the following modifications were made:

Introduced a sync.Mutex in the hooksMixin struct to ensure thread-safe operations. Implemented locking and unlocking around critical sections in AddHook and dialHook methods to prevent concurrent read/write conflicts. Adjusted the clone method in hooksMixin to handle the mutex correctly, avoiding the copying of mutex state.

I invite feedback and further review on these changes!

RyoMiyashita commented 9 months ago

@ofekshenawa I'm in a bit of a rush to get it merged – it's pretty critical for our current project. Any chance you could take a peek soon? Would really appreciate a speedy review if possible!

ofekshenawa commented 9 months ago

Hey @RyoMiyashita ! I totally get the urgency. Just finished reviewing the changes and everything looks solid. I’m going to merge this into master now.

RyoMiyashita commented 9 months ago

@ofekshenawa Thanks for taking the time to review and merge this. Really appreciate it!