DATA RACE in resty.(*Client).execute()

[iyidan]

Hello,
I am use resty in multi-goroutine and case the DATA RACE

test.go
`package main

import (
"github.com/go-resty/resty"
"sync"
)

func main() {
w := sync.WaitGroup{}
for i := 0; i < 50; i++ {
w.Add(1)
go func() {
defer w.Done()
resty.R().Get("http://httpbin.org/get")
}()
}
w.Wait()
} go run -race test.go`

[jeevatkm]

Thanks for reporting. I will have a look.

[jeevatkm]

@iyidan I have analyzed it, will be available in next version. If you have PR for this, feel free to send it.

[jeevatkm]

@iyidan - I have taken care of it, however will do improvements later.

[jeevatkm]

It will be part of v0.8 release, available in master branch.

[mrd0ll4r]

Sorry to resurrect this, just to let you know: This has broken my client as resty is not parallel anymore. I knew about the data race but it was not affecting my use case. I can fix it without much issues, but please be careful in the future about changes that change the behavior of resty so much. Thanks for the library, will keep using it! :)

[jeevatkm]

@mrd0ll4r I'm sorry about you ran into issue.

Please let me know your suggestion to improve resty. I will think about keeping parallel and prevent data race in the mean time.

Looking forward to your inputs.

[mrd0ll4r]

Hard to say - you could use an RWMutex for the proxy settings etc, but you allow setting a proxy for an individual Request, so that's not going to work. Maybe make it so that requests with an explicit proxy use a separate transport and HTTP client? Hard to say for sure.

You could always just say "resty is not thread safe when modifying proxy settings (and maybe more), do thread safety in your code", which works fine for me.

[jeevatkm]

@mrd0ll4r @wanghq - I thought through it. I'm planning to remove request level proxy option and mutex locking from resty.

any objections or comment?

[mrd0ll4r]

You'd still protect the top-level proxy with an RWMutex or something, right? Sounds good to me.

[jeevatkm]

@mrd0ll4r that's the plan, I'm gonna give it try.

[jeevatkm]

@mrd0ll4r @wanghp - mutex lock is removed from resty. I have tried RWMutex however it is not needed anymore.

I need to add note for, if resty user is modifying resty client setting from multiple goroutine/dynamically they have to use their own locking mechanism.

I have pushed changes to master. resty Performance is improved.

[wanghq]

@jeevatkm , thanks for the quick response! Looks good.

[jeevatkm]

@wanghq have you tested master at your end. Please let me know your feedback.

[wanghq]

@jeevatkm , I didn't notice any issue with the master branch. Thanks again.

[jeevatkm]

@wanghq Thank you for the confirmation.