Default number of network retries to 2 (#1069)

Currently, it's possible to have the library retry intermittently failed
requests by configuring `MaxNetworkRetries` on a `BackendConfig`.
Because this is a pretty useful feature that many users will never
discover because it's off by default, we've been mulling around the idea
internally to change that default on the next major so that more people
get access to it. We're about to release V71, so now is an opportune
moment.

The slight complication is that `BackendConfig.MaxNetworkRetries` is
currently a simple `int`, which means that it's hard for us to recognize
an unset value versus an explicitly set 0 (the same problem we had with
fields on parameter structs for years). So by example, this code is
problematic:

``` go
if conf.MaxNetworkRetries == 0 {
    backend.MaxNetworkRetries = 2
}
```

The most obvious solution is that change `MaxNetworkRetries` to a
nilable pointer, and have it set using our `stripe.Int64` helper,
exactly as we do for parameter structs. So compared to today,
configuring it would change like this:

``` patch
 config := &stripe.BackendConfig{
-    MaxNetworkRetries: 2,
+    MaxNetworkRetries: stripe.Int64(2),
 }
```

It's not too bad, and follows convention found elsewhere in the library,
but will require a small code update for users.

The slight follow on complication is that to make `BackendConfig`
self-consistent, I also changed the other primitives on it to also be
pointers, so `EnableTelemetry` changes from `bool` to `*bool` and `URL`
changes from `string` to `*string`. I don't think this is a big deal
because ~99% of users will probably just be using the defaults by having
left them unset.

README.md

@@ -237,10 +237,15 @@ if err := i.Err(); err != nil {
 }
 ```
 
-### Configuring Automatic Retries
+### Automatic Retries
 
-You can enable automatic retries on requests that fail due to a transient
-problem by configuring the maximum number of retries:
+The library automatically retries requests on intermittent failures like on a
+connection error, timeout, or on certain API responses like a status `409
+Conflict`. [Idempotency keys][idempotency-keys] are always added to requests to
+make any such subsequent retries safe.
+
+By default, it will perform up to two retries. That number can be configured
+with `MaxNetworkRetries`:
 
 ```go
 import (
@@ -249,7 +254,7 @@ import (
 )
 
 config := &stripe.BackendConfig{
-    MaxNetworkRetries: 2,
+    MaxNetworkRetries: stripe.Int64(0), // Zero retries
 }
 
 sc := &client.API{}
@@ -261,12 +266,6 @@ sc.Init("sk_key", &stripe.Backends{
 coupon, err := sc.Coupons.New(...)
 ```
 
-Various errors can trigger a retry, like a connection error or a timeout, and
-also certain API responses like HTTP status `409 Conflict`.
-
-[Idempotency keys][idempotency-keys] are added to requests to guarantee that
-retries are safe.
-
 ### Configuring Logging
 
 By default, the library logs error messages only (which are sent to `stderr`).
@@ -331,7 +330,7 @@ You can disable this behavior if you prefer:
 
 ```go
 config := &stripe.BackendConfig{
-	EnableTelemetry: false,
+	EnableTelemetry: stripe.Bool(false),
 }
 ```
 

error_test.go

@@ -23,7 +23,7 @@ func TestErrorResponse(t *testing.T) {
 	defer ts.Close()
 
 	backend := GetBackendWithConfig(APIBackend, &BackendConfig{
-		URL: ts.URL,
+		URL: String(ts.URL),
 	})
 
 	err := backend.Call(http.MethodGet, "/v1/account", "sk_test_badKey", nil, nil)

oauth/client_test.go

@@ -121,7 +121,7 @@ func stubConnectBackend(httpClient *http.Client) {
 	mockBackend := stripe.GetBackendWithConfig(
 		stripe.ConnectBackend,
 		&stripe.BackendConfig{
-			URL:        "https://localhost:12113",
+			URL:        stripe.String("https://localhost:12113"),
 			HTTPClient: httpClient,
 		},
 	)

order/client_test.go

@@ -64,7 +64,7 @@ func TestOrderReturn_RequestParams(t *testing.T) {
 
 	// Configure the stripe client to use the ephemeral backend.
 	backend := stripe.GetBackendWithConfig(stripe.APIBackend, &stripe.BackendConfig{
-		URL: ts.URL,
+		URL: stripe.String(ts.URL),
 	})
 	orderClient := Client{B: backend, Key: stripe.Key}
 

stripe.go

@@ -43,6 +43,10 @@ const (
 	// OAuth.
 	ConnectBackend SupportedBackend = "connect"
 
+	// DefaultMaxNetworkRetries is the default maximum number of retries made
+	// by a Stripe client.
+	DefaultMaxNetworkRetries int64 = 2
+
 	// UnknownPlatform is the string returned as the system name if we couldn't get
 	// one from `uname`.
 	UnknownPlatform string = "unknown platform"
@@ -152,16 +156,19 @@ type Backend interface {
 	Call(method, path, key string, params ParamsContainer, v LastResponseSetter) error
 	CallRaw(method, path, key string, body *form.Values, params *Params, v LastResponseSetter) error
 	CallMultipart(method, path, key, boundary string, body *bytes.Buffer, params *Params, v LastResponseSetter) error
-	SetMaxNetworkRetries(maxNetworkRetries int)
+	SetMaxNetworkRetries(maxNetworkRetries int64)
 }
 
 // BackendConfig is used to configure a new Stripe backend.
 type BackendConfig struct {
 	// EnableTelemetry allows request metrics (request id and duration) to be sent
 	// to Stripe in subsequent requests via the `X-Stripe-Client-Telemetry` header.
 	//
+	// This value is a pointer to allow us to differentiate an unset versus
+	// empty value. Use stripe.Bool for an easy way to set this value.
+	//
 	// Defaults to false.
-	EnableTelemetry bool
+	EnableTelemetry *bool
 
 	// HTTPClient is an HTTP client instance to use when making API requests.
 	//
@@ -188,13 +195,19 @@ type BackendConfig struct {
 	// retry requests that appear to have failed due to an intermittent
 	// problem.
 	//
-	// Defaults to 0.
-	MaxNetworkRetries int
+	// This value is a pointer to allow us to differentiate an unset versus
+	// empty value. Use stripe.Int64 for an easy way to set this value.
+	//
+	// Defaults to DefaultMaxNetworkRetries (2).
+	MaxNetworkRetries *int64
 
 	// URL is the base URL to use for API paths.
 	//
+	// This value is a pointer to allow us to differentiate an unset versus
+	// empty value. Use stripe.String for an easy way to set this value.
+	//
 	// If left empty, it'll be set to the default for the SupportedBackend.
-	URL string
+	URL *string
 }
 
 // BackendImplementation is the internal implementation for making HTTP calls
@@ -207,7 +220,7 @@ type BackendImplementation struct {
 	URL               string
 	HTTPClient        *http.Client
 	LeveledLogger     LeveledLoggerInterface
-	MaxNetworkRetries int
+	MaxNetworkRetries int64
 
 	enableTelemetry bool
 
@@ -557,7 +570,7 @@ func (s *BackendImplementation) ResponseToError(res *http.Response, resBody []by
 // SetMaxNetworkRetries sets max number of retries on failed requests
 //
 // This function is deprecated. Please use GetBackendWithConfig instead.
-func (s *BackendImplementation) SetMaxNetworkRetries(maxNetworkRetries int) {
+func (s *BackendImplementation) SetMaxNetworkRetries(maxNetworkRetries int64) {
 	s.MaxNetworkRetries = maxNetworkRetries
 }
 
@@ -600,7 +613,7 @@ func (s *BackendImplementation) UnmarshalJSONVerbose(statusCode int, body []byte
 // socket errors that may represent an intermittent problem and some special
 // HTTP statuses.
 func (s *BackendImplementation) shouldRetry(err error, req *http.Request, resp *http.Response, numRetries int) bool {
-	if numRetries >= s.MaxNetworkRetries {
+	if numRetries >= int(s.MaxNetworkRetries) {
 		return false
 	}
 
@@ -800,8 +813,8 @@ func GetBackend(backendType SupportedBackend) Backend {
 		&BackendConfig{
 			HTTPClient:        httpClient,
 			LeveledLogger:     nil, // Set by GetBackendWithConfiguation when nil
-			MaxNetworkRetries: 0,
-			URL:               "", // Set by GetBackendWithConfiguation when empty
+			MaxNetworkRetries: nil, // Set by GetBackendWithConfiguation when nil
+			URL:               nil, // Set by GetBackendWithConfiguation when nil
 		},
 	)
 
@@ -822,31 +835,35 @@ func GetBackendWithConfig(backendType SupportedBackend, config *BackendConfig) B
 		config.LeveledLogger = DefaultLeveledLogger
 	}
 
+	if config.MaxNetworkRetries == nil {
+		config.MaxNetworkRetries = Int64(DefaultMaxNetworkRetries)
+	}
+
 	switch backendType {
 	case APIBackend:
-		if config.URL == "" {
-			config.URL = apiURL
+		if config.URL == nil {
+			config.URL = String(apiURL)
 		}
 
-		config.URL = normalizeURL(config.URL)
+		config.URL = String(normalizeURL(*config.URL))
 
 		return newBackendImplementation(backendType, config)
 
 	case UploadsBackend:
-		if config.URL == "" {
-			config.URL = uploadsURL
+		if config.URL == nil {
+			config.URL = String(uploadsURL)
 		}
 
-		config.URL = normalizeURL(config.URL)
+		config.URL = String(normalizeURL(*config.URL))
 
 		return newBackendImplementation(backendType, config)
 
 	case ConnectBackend:
-		if config.URL == "" {
-			config.URL = ConnectURL
+		if config.URL == nil {
+			config.URL = String(ConnectURL)
 		}
 
-		config.URL = normalizeURL(config.URL)
+		config.URL = String(normalizeURL(*config.URL))
 
 		return newBackendImplementation(backendType, config)
 	}
@@ -1139,8 +1156,12 @@ func isHTTPWriteMethod(method string) bool {
 // The vast majority of the time you should be calling GetBackendWithConfig
 // instead of this function.
 func newBackendImplementation(backendType SupportedBackend, config *BackendConfig) Backend {
+	enableTelemetry := EnableTelemetry
+	if config.EnableTelemetry != nil {
+		enableTelemetry = *config.EnableTelemetry
+	}
+
 	var requestMetricsBuffer chan requestMetrics
-	enableTelemetry := config.EnableTelemetry || EnableTelemetry
 
 	// only allocate the requestMetrics buffer if client telemetry is enabled.
 	if enableTelemetry {
@@ -1150,9 +1171,9 @@ func newBackendImplementation(backendType SupportedBackend, config *BackendConfi
 	return &BackendImplementation{
 		HTTPClient:           config.HTTPClient,
 		LeveledLogger:        config.LeveledLogger,
-		MaxNetworkRetries:    config.MaxNetworkRetries,
+		MaxNetworkRetries:    *config.MaxNetworkRetries,
 		Type:                 backendType,
-		URL:                  config.URL,
+		URL:                  *config.URL,
 		enableTelemetry:      enableTelemetry,
 		networkRetriesSleep:  true,
 		requestMetricsBuffer: requestMetricsBuffer,

stripe_test.go

@@ -101,8 +101,8 @@ func TestDo_Retry(t *testing.T) {
 		APIBackend,
 		&BackendConfig{
 			LeveledLogger:     debugLeveledLogger,
-			MaxNetworkRetries: 5,
-			URL:               testServer.URL,
+			MaxNetworkRetries: Int64(5),
+			URL:               String(testServer.URL),
 		},
 	).(*BackendImplementation)
 
@@ -130,12 +130,12 @@ func TestDo_Retry(t *testing.T) {
 }
 
 func TestShouldRetry(t *testing.T) {
-	MaxNetworkRetries := 3
+	MaxNetworkRetries := int64(3)
 
 	c := GetBackendWithConfig(
 		APIBackend,
 		&BackendConfig{
-			MaxNetworkRetries: MaxNetworkRetries,
+			MaxNetworkRetries: Int64(MaxNetworkRetries),
 		},
 	).(*BackendImplementation)
 
@@ -144,7 +144,7 @@ func TestShouldRetry(t *testing.T) {
 		nil,
 		&http.Request{},
 		&http.Response{},
-		MaxNetworkRetries,
+		int(MaxNetworkRetries),
 	))
 
 	// Doesn't retry most Stripe errors (they must also match a status code
@@ -261,8 +261,8 @@ func TestDo_RetryOnTimeout(t *testing.T) {
 		APIBackend,
 		&BackendConfig{
 			LeveledLogger:     debugLeveledLogger,
-			MaxNetworkRetries: 1,
-			URL:               testServer.URL,
+			MaxNetworkRetries: Int64(1),
+			URL:               String(testServer.URL),
 			HTTPClient:        &http.Client{Timeout: timeout},
 		},
 	).(*BackendImplementation)
@@ -314,8 +314,8 @@ func TestDo_LastResponsePopulated(t *testing.T) {
 		APIBackend,
 		&BackendConfig{
 			LeveledLogger:     debugLeveledLogger,
-			MaxNetworkRetries: 0,
-			URL:               testServer.URL,
+			MaxNetworkRetries: Int64(0),
+			URL:               String(testServer.URL),
 		},
 	).(*BackendImplementation)
 
@@ -373,8 +373,8 @@ func TestDo_TelemetryDisabled(t *testing.T) {
 		APIBackend,
 		&BackendConfig{
 			LeveledLogger:     debugLeveledLogger,
-			MaxNetworkRetries: 0,
-			URL:               testServer.URL,
+			MaxNetworkRetries: Int64(0),
+			URL:               String(testServer.URL),
 		},
 	).(*BackendImplementation)
 
@@ -461,10 +461,10 @@ func TestDo_TelemetryEnabled(t *testing.T) {
 	backend := GetBackendWithConfig(
 		APIBackend,
 		&BackendConfig{
+			EnableTelemetry:   Bool(true),
 			LeveledLogger:     debugLeveledLogger,
-			MaxNetworkRetries: 0,
-			URL:               testServer.URL,
-			EnableTelemetry:   true,
+			MaxNetworkRetries: Int64(0),
+			URL:               String(testServer.URL),
 		},
 	).(*BackendImplementation)
 
@@ -519,10 +519,10 @@ func TestDo_TelemetryEnabledNoDataRace(t *testing.T) {
 	backend := GetBackendWithConfig(
 		APIBackend,
 		&BackendConfig{
+			EnableTelemetry:   Bool(true),
 			LeveledLogger:     debugLeveledLogger,
-			MaxNetworkRetries: 0,
-			URL:               testServer.URL,
-			EnableTelemetry:   true,
+			MaxNetworkRetries: Int64(0),
+			URL:               String(testServer.URL),
 		},
 	).(*BackendImplementation)
 
@@ -584,7 +584,7 @@ func TestGetBackendWithConfig_TrimV1Suffix(t *testing.T) {
 		backend := GetBackendWithConfig(
 			APIBackend,
 			&BackendConfig{
-				URL: "https://api.com/v1",
+				URL: String("https://api.com/v1"),
 			},
 		).(*BackendImplementation)
 
@@ -598,7 +598,7 @@ func TestGetBackendWithConfig_TrimV1Suffix(t *testing.T) {
 		backend := GetBackendWithConfig(
 			APIBackend,
 			&BackendConfig{
-				URL: "https://api.com/v1/",
+				URL: String("https://api.com/v1/"),
 			},
 		).(*BackendImplementation)
 
@@ -610,7 +610,7 @@ func TestGetBackendWithConfig_TrimV1Suffix(t *testing.T) {
 		backend := GetBackendWithConfig(
 			APIBackend,
 			&BackendConfig{
-				URL: "https://api.com",
+				URL: String("https://api.com"),
 			},
 		).(*BackendImplementation)
 

testing/testing.go

@@ -90,7 +90,7 @@ func init() {
 	stripeMockBackend := stripe.GetBackendWithConfig(
 		stripe.APIBackend,
 		&stripe.BackendConfig{
-			URL:           "https://localhost:" + port,
+			URL:           stripe.String("https://localhost:" + port),
 			HTTPClient:    httpClient,
 			LeveledLogger: stripe.DefaultLeveledLogger,
 		},