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 go vet and go test -race #35

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ashanbrown
Copy link

Here are a few fixes to satisfy go vet and test. I'm running these on circleci. We use them regularly and it might be worth enabling them for this repo (maybe even adding a badge). There is also travis if you'd prefer that. You can see the failures this fixes at:

https://circleci.com/gh/launchdarkly/eventsource/19

go vet notes that using non-pointer methods copies the mutext lock that is a member of this class.
Otherwise go test -race may report that checkRedirect is read before it is written.
@ashanbrown ashanbrown changed the title fix go vet and test fix go vet and go test -race Oct 31, 2017
@donovanhide
Copy link
Owner

Looks good! Bit worried about the race condition fix, as users who had counted on SubscribeWith() following redirects will now have to configure their http.Client themselves:

6fc0f23#diff-e2d91404d5609adc9273e4603f6ce67dL73

@ashanbrown
Copy link
Author

I moved the redirecting logic into a new redirectingClient, which I believe should be used instead of the defaultClient. I don't think the user has to set up anything. I think the one change is that we haven't altered the behavior of defaultClient everywhere else it is used in the application/library.

@donovanhide
Copy link
Owner

If a user has created a Stream with SubscribeWith and provided their own http.Client, then with this patch, the Client will not have its CheckRedirect func set to our checkRedirect handler, which could break user code which depends on all the headers being copied. This seems more subtle than you might expect:
golang/go#4800
Setting the CheckRedirect func in library code is inherently racy... One solution is to export the RedirectingClient and document its use, but I'm not aware of the exact situations when it should be used, although this commit gives some examples in the commit message:
aee4cf0

@donovanhide
Copy link
Owner

Actually seems like copying headers on redirect was added in Go 1.8:

https://golang.org/doc/go1.8#net_http

Tricky decision is whether to remove the redirect functionality, and document Go 1.8 as a minimum requirement or to leave in backwards compatibility.

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

Successfully merging this pull request may close these issues.

2 participants