Repository: Shopify/themekit (go)

Issue:
theme download gets stuck at 97%
Hi there,

I recently updated themekit to latest version (1.1.1) and since then it cannot download all the files...
I tried with 3 different themes and all of them get stuck around 97-98%

`[development] Warning, this is the live theme on XXXXX-THEME.`
`[development] 389|401 [===================================================================>--]  97 %`

Any thoughts on this?

Thanks

**OS**: Mac
**Themekit**: v1.1.1

Pull request:
Http Client tweaks to idle conns to prevent hangs in connections
will-fix #801 

With my experience doing highly concurrent http request in Go, I know that the default client has issues managing idle connection management, especially if it has no bounds on growth. I believe that this may be the issue that is causing the requests to hang. I debugged the hanging issues and the hang was actually happening on the request.

@MuhammadFarag I pinged you because you made the last tweaks to CH http client

### Warn Checklist
- [ ] This changes the interface and requires a Major/Minor version change.
- [x] I have :tophat:'d these changes by using the commands I changed by hand.
- [ ] I have added a dependancy to the project.
- [x] I have considered any potential impact on [node-themekit](https://github.com/Shopify/node-themekit)


Patch:
diff --git a/src/env/env.go b/src/env/env.go
index 55510dc..3b92be6 100644
--- a/src/env/env.go
+++ b/src/env/env.go
@@ -28,8 +28,7 @@ type Env struct {
 
 //Default is the default values for a environment
 var Default = Env{
-	Name:    "development",
-	Timeout: 30 * time.Second,
+	Name: "development",
 }
 
 func init() {
diff --git a/src/httpify/client.go b/src/httpify/client.go
index d2c9ef0..34b7e8b 100644
--- a/src/httpify/client.go
+++ b/src/httpify/client.go
@@ -2,11 +2,13 @@ package httpify
 
 import (
 	"bytes"
+	"context"
 	"crypto/tls"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"io/ioutil"
+	"net"
 	"net/http"
 	"net/url"
 	"runtime"
@@ -19,9 +21,39 @@ import (
 )
 
 var (
-	errConnectionIssue = errors.New("DNS problem while connecting to Shopify, this indicates a problem with your internet connection")
+	// ErrConnectionIssue is an error that is thrown when a very specific error is
+	// returned from our http request that usually implies bad connections.
+	ErrConnectionIssue = errors.New("DNS problem while connecting to Shopify, this indicates a problem with your internet connection")
+	// ErrInvalidProxyURL is returned if a proxy url has been passed but is improperly formatted
+	ErrInvalidProxyURL = errors.New("invalid proxy URI")
+	netDialer          = &net.Dialer{
+		Timeout:   3 * time.Second,
+		KeepAlive: 1 * time.Second,
+	}
+	httpTransport = &http.Transport{
+		TLSClientConfig:       &tls.Config{InsecureSkipVerify: true},
+		IdleConnTimeout:       time.Second,
+		TLSHandshakeTimeout:   time.Second,
+		ExpectContinueTimeout: time.Second,
+		ResponseHeaderTimeout: time.Second,
+		MaxIdleConnsPerHost:   10,
+		DialContext: func(ctx context.Context, network, address string) (conn net.Conn, err error) {
+			if conn, err = netDialer.DialContext(ctx, network, address); err != nil {
+				return nil, err
+			}
+			deadline := time.Now().Add(5 * time.Second)
+			conn.SetReadDeadline(deadline)
+			return conn, conn.SetDeadline(deadline)
+		},
+	}
+	httpClient = &http.Client{
+		Transport: httpTransport,
+		Timeout:   30 * time.Second,
+	}
 )
 
+type proxyHandler func(*http.Request) (*url.URL, error)
+
 // Params allows for a better structured input into NewClient
 type Params struct {
 	Domain   string
@@ -36,7 +68,6 @@ type HTTPClient struct {
 	domain   string
 	password string
 	baseURL  *url.URL
-	client   *http.Client
 	limit    *ratelimiter.Limiter
 	maxRetry int
 }
@@ -49,16 +80,22 @@ func NewClient(params Params) (*HTTPClient, error) {
 		return nil, err
 	}
 
-	adapter, err := generateHTTPAdapter(params.Timeout, params.Proxy)
-	if err != nil {
-		return nil, err
+	if params.Timeout != 0 {
+		httpClient.Timeout = params.Timeout
+	}
+
+	if params.Proxy != "" {
+		parsedURL, err := url.ParseRequestURI(params.Proxy)
+		if err != nil {
+			return nil, ErrInvalidProxyURL
+		}
+		httpTransport.Proxy = http.ProxyURL(parsedURL)
 	}
 
 	return &HTTPClient{
 		domain:   params.Domain,
 		password: params.Password,
 		baseURL:  baseURL,
-		client:   adapter,
 		limit:    ratelimiter.New(params.Domain, 4),
 		maxRetry: 5,
 	}, nil
@@ -105,8 +142,7 @@ func (client *HTTPClient) do(method, path string, body interface{}, headers map[
 }
 
 func (client *HTTPClient) doWithRetry(req *http.Request, body interface{}) (*http.Response, error) {
-	attempt := 0
-	for {
+	for attempt := 0; attempt <= client.maxRetry; {
 		// reset the body when non-nil for every request (rewind)
 		if body != nil {
 			data, err := json.Marshal(body)
@@ -117,51 +153,22 @@ func (client *HTTPClient) doWithRetry(req *http.Request, body interface{}) (*htt
 		}
 
 		client.limit.Wait()
-		resp, err := client.client.Do(req)
+		resp, err := httpClient.Do(req)
 		if err == nil {
 			if resp.StatusCode >= 100 && resp.StatusCode <= 428 {
 				return resp, nil
 			} else if resp.StatusCode == http.StatusTooManyRequests {
 				after, _ := strconv.ParseFloat(resp.Header.Get("Retry-After"), 10)
-				client.limit.ResetAfter(time.Duration(after))
+				client.limit.ResetAfter(time.Duration(after) * time.Second)
 				continue
 			}
 		} else if strings.Contains(err.Error(), "no such host") {
-			return nil, errConnectionIssue
+			return nil, ErrConnectionIssue
 		}
-
 		attempt++
-		if attempt > client.maxRetry {
-			return resp, fmt.Errorf("request failed after %v retries with err: %v", client.maxRetry, err)
-		}
 		time.Sleep(time.Duration(attempt) * time.Second)
 	}
-}
-
-func generateHTTPAdapter(timeout time.Duration, proxyURL string) (*http.Client, error) {
-	adapter := &http.Client{Timeout: timeout}
-	if transport, err := generateClientTransport(proxyURL); err != nil {
-		return nil, err
-	} else if transport != nil {
-		adapter.Transport = transport
-	}
-	return adapter, nil
-}
-
-func generateClientTransport(proxyURL string) (*http.Transport, error) {
-	if proxyURL == "" {
-		return nil, nil
-	}
-
-	parsedURL, err := url.ParseRequestURI(proxyURL)
-	if err != nil {
-		return nil, fmt.Errorf("invalid proxy URI")
-	}
-
-	return &http.Transport{
-		Proxy:           http.ProxyURL(parsedURL),
-		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
-	}, nil
+	return nil, fmt.Errorf("request failed after %v retries", client.maxRetry)
 }
 
 func parseBaseURL(domain string) (*url.URL, error) {
diff --git a/src/ratelimiter/rate_limiter.go b/src/ratelimiter/rate_limiter.go
index 1981fe0..1e29557 100644
--- a/src/ratelimiter/rate_limiter.go
+++ b/src/ratelimiter/rate_limiter.go
@@ -20,7 +20,7 @@ func New(domain string, reqPerSec int) *Limiter {
 		everySecond := rate.Every(time.Second / time.Duration(reqPerSec))
 		domainLimitMap[domain] = &Limiter{
 			perSecond: everySecond,
-			rate:      rate.NewLimiter(everySecond, 10),
+			rate:      rate.NewLimiter(everySecond, reqPerSec),
 		}
 	}
 	return domainLimitMap[domain]
diff --git a/src/release/release.go b/src/release/release.go
index 87359cc..9483b2d 100644
--- a/src/release/release.go
+++ b/src/release/release.go
@@ -30,7 +30,7 @@ var (
 		"windows-amd64": "theme.exe",
 	}
 	// ThemeKitVersion is the version build of the library
-	ThemeKitVersion, _ = version.NewVersion("1.1.2")
+	ThemeKitVersion, _ = version.NewVersion("1.1.3")
 )
 
 const (


Test patch:
diff --git a/src/httpify/client_test.go b/src/httpify/client_test.go
index 032e7a4..d9a9cff 100644
--- a/src/httpify/client_test.go
+++ b/src/httpify/client_test.go
@@ -139,37 +139,41 @@ func TestClient_do(t *testing.T) {
 	assert.Nil(t, err)
 
 	_, err = client.do("POST", "/assets.json", body, nil)
-	assert.Contains(t, err.Error(), "request failed after 1 retries with err: ", server.URL)
+	assert.Contains(t, err.Error(), "request failed after 1 retries", server.URL)
 	server.Close()
 }
 
 func TestGenerateHTTPAdapter(t *testing.T) {
-	_, err := generateHTTPAdapter(time.Second, "#$#$^$%^##$")
-	if assert.NotNil(t, err) {
-		assert.EqualError(t, err, "invalid proxy URI")
-	}
+	NewClient(Params{
+		Domain:  "https://shop.myshopify.com",
+		Timeout: 60 * time.Second,
+	})
+	assert.Equal(t, httpClient.Timeout, 60*time.Second)
 
-	c, err := generateHTTPAdapter(time.Second, "http://localhost:3000")
-	assert.Nil(t, err)
-	assert.Equal(t, time.Second, c.Timeout)
-	assert.NotNil(t, c.Transport)
+	NewClient(Params{Domain: "https://shop.myshopify.com"})
+	assert.Equal(t, httpClient.Timeout, 60*time.Second)
 }
 
-func TestGenerateClientTransport(t *testing.T) {
+func TestProxyConfig(t *testing.T) {
 	testcases := []struct {
 		proxyURL, err string
-		expectNil     bool
 	}{
-		{proxyURL: "", expectNil: true},
-		{proxyURL: "http//localhost:3000", expectNil: true, err: "invalid proxy URI"},
-		{proxyURL: "http://127.0.0.1:8080", expectNil: false},
+		{proxyURL: ""},
+		{proxyURL: "http//localhost:3000", err: "invalid proxy URI"},
+		{proxyURL: "http://127.0.0.1:8080"},
 	}
 
 	for _, testcase := range testcases {
-		transport, err := generateClientTransport(testcase.proxyURL)
-		assert.Equal(t, transport == nil, testcase.expectNil)
-		if testcase.err == "" {
-			assert.Nil(t, err)
+		_, err := NewClient(Params{
+			Domain: "https://shop.myshopify.com",
+			Proxy:  testcase.proxyURL,
+		})
+		if testcase.err == "" && assert.Nil(t, err) {
+			if testcase.proxyURL == "" {
+				assert.Nil(t, httpTransport.Proxy)
+			} else {
+				assert.NotNil(t, httpTransport.Proxy)
+			}
 		} else if assert.NotNil(t, err) {
 			assert.Contains(t, err.Error(), testcase.err)
 		}