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

httpClient.Do is protected by a mutex? #56

Closed
wanghq opened this issue Feb 9, 2017 · 3 comments
Closed

httpClient.Do is protected by a mutex? #56

wanghq opened this issue Feb 9, 2017 · 3 comments
Assignees

Comments

@wanghq
Copy link

wanghq commented Feb 9, 2017

Because of the locking, there can only be one outgoing call, which is really bad to the performance. If I understand it correctly,

  • if req1 has proxyURL set but req2 doesn't, then req2 will be sent with req1's proxy. Is this expected?
  • if req1 and req2 both have proxyURL set, then req1 will be sent with req1's proxy and req2 will be sent with req2's proxy. This works. But the overhead of this is to not have more than 1 outgoing calls.
  • if Unlock() is moved up before httpClient.Do. For the above case, there is no guarantee on which proxy will be used, which is not good.

I hope this per request proxy is an optional feature. Providing it is fine but without sacrificing the normal use case.

Code ref:
https://github.com/go-resty/resty/blob/master/client.go#L634-L647

	c.mutex.Lock()

	if req.proxyURL != nil {
		c.transport.Proxy = http.ProxyURL(req.proxyURL)
	} else if c.proxyURL != nil {
		c.transport.Proxy = http.ProxyURL(c.proxyURL)
	}

	req.Time = time.Now()
	c.httpClient.Transport = c.transport

	resp, err := c.httpClient.Do(req.RawRequest)

        c.mutex.Unlock()
@jeevatkm jeevatkm self-assigned this Feb 9, 2017
@jeevatkm
Copy link
Member

jeevatkm commented Feb 9, 2017

I understand what you're describing. I have introduced locking in #29. but I did not satisfied myself.

It seems I have to make a call before v1.0 release on this locking.

@jeevatkm
Copy link
Member

@wanghq mutex removed, request level proxy setting is removed. Locking is not needed anymore for resty.

Refer #29 too.

@jeevatkm
Copy link
Member

Updates are present in #29. Closing this one.

wppurking added a commit to wppurking/resty that referenced this issue Dec 4, 2017
according to go-resty#56  the Request Level Proxy is removed.
jeevatkm pushed a commit that referenced this issue Dec 4, 2017
according to #56  the Request Level Proxy is removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants