Make client.SetHeader() thread safe

[amarjeetanandsingh]

Hi

Is there any problem to make client.SetHeader() thread safe? Any specific reason?

If performance impact is a reason, should we consider making it configurable or expose a safe api like client.SetHeaderSafe()?

@jeevatkm

[jeevatkm]

@amarjeetanandsingh Thanks for reaching out. Ideally, you create a Resty client once in the beginning, and requests made using that client across for the purpose.

Can you please share the scenario and your usage of the resty client? An example will help me to understand your usage.

[amarjeetanandsingh]

Thanks for your response @jeevatkm

Agree that we should be creating resty client only once, but there can be a situation where we need to add one more request header later(based on some condition) to make this header available in every next request through this client. At that time we may run into race condition while setting up the new header.

If we don't want to update the header in resty client, another solution is to store the newHeader value somewhere in the global variable and add that in all the subsequent requests, which I feel wll be a lot more hassle.

NOTE: Since resty client creation is a one time job, I feel making it thread safe wouldn't impact the performance.

[jeevatkm]

@amarjeetanandsingh I'm trying to get all issues and find closure. One issue I see here is Client.Header exported field, having a write sync lock may not be helpful. I think it's better to apply at your end where you have client instances created and manipulated.

[ben548]

I am facing a similar issue where I need to set an authorization header for each HTTP request I make. Upon examining the code, I realized that the SetHeader function is not thread-safe since it uses a map[string][]string as its data structure and I did not see any locking mechanism in place.

In this case, I am wondering how I can make it thread-safe or if I should consider using a different HTTP request package from GitHub.

[jeevatkm]

I'm considering using Mutex on the v3.0.0 at the client object level. Let's see the behavior afterward.

[amarjeetanandsingh]

@jeevatkm May I deliver this fix?

[SVilgelm]

@amarjeetanandsingh The PR you created fixes the only issue with writing the headers, but what will happen if another thread is reading those headers at same time?

In my opinion, there is no need in using the mutexes to lock some particular resources in the client. It should be a global lock for any client properties. Also with the current code this will be very hard, since most of the properties are public.

In your case I would use a global mutex and use it everywhere in your application.

[amarjeetanandsingh]

Yes, @SVilgelm. The change needs a discussion to conclude. IMO there is no straightforward way to make that thread-safe, the main reason being the exported variables.

However if we want to go ahead making it thread-safe, I feel we need to consider other APIs as well.

[jeevatkm]

@amarjeetanandsingh FYI, https://github.com/go-resty/resty/pull/717#issuecomment-1742211834

[zliang-akamai]

Would love to see this feature comes to reality 😍

[jeevatkm]

Resty v3 Client has a sync.RWMutex to make property modification concurrent-safe. Currently, the development version is available on the branch main.