Skip to content
This repository has been archived by the owner on Aug 31, 2018. It is now read-only.

Data race warning #111

Open
Perlence opened this issue Aug 4, 2015 · 9 comments
Open

Data race warning #111

Perlence opened this issue Aug 4, 2015 · 9 comments

Comments

@Perlence
Copy link

Perlence commented Aug 4, 2015

I use goreq to send concurrent requests:

resp, err := goreq.Request{
    Uri: "https://en.wikipedia.org/w/api.php",
    QueryString: url.Values{
        "action": {"opensearch"},
        "search": {query},
    },
}.Do()

But when I checked the program for races, I got this:

WARNING: DATA RACE
Write by goroutine 14:
  github.com/franela/goreq.Request.Do()
      /home/ubuntu/.local/go/src/github.com/franela/goreq/goreq.go:314 +0x6ff

So race detector warns me that goroutines write unsafely into CheckRedirect of DefaultClient.

@marcosnils
Copy link
Member

@Perlence can you update goreq and run the race detector again?. Seems like is something else https://github.com/franela/goreq/blob/master/goreq.go#L314

@Perlence
Copy link
Author

Perlence commented Aug 4, 2015

I've updated goreq so the master is 8f5efc2. Unfortunately warning's still there:

Write by goroutine 14:
  github.com/franela/goreq.Request.Do()
      /home/ubuntu/.local/go/src/github.com/franela/goreq/goreq.go:323 +0x6ff

@marcosnils
Copy link
Member

@Perlence We found the issue. We'll be fixing this shorty. It won't affect your code unless you are specifying different maxRedirects per request.

@rhd
Copy link

rhd commented Sep 25, 2015

Hi, I'm seeing the same issue - would love a fix just to clean up the warning.

@marcosnils
Copy link
Member

@rhd we'll fix it shortly.

@rrh
Copy link

rrh commented Oct 9, 2015

I have independently rediscovered this race condition in my application. I'm at head, eg 72c51a5

Note also that changing the Makefile to run go test --race finds other race conditions as well.

@sschepens
Copy link
Contributor

Yes, there are some race conditions they all come from using DefaultClient and allowing functionallity that should require a new client.
I believe we should add a Client abstraction to goreq and make these things configurable at a global level and allow to create new goreq clients.

@rrh
Copy link

rrh commented Oct 12, 2015

any progress on this over the weekend?

@sschepens
Copy link
Contributor

@rrh I haven't had time to check this, but those races are hard to tackle without breaking backwards compatibility, that's a bummer.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants