From 74f3fd00fd426d15c47e02e9a0610184aab2497d Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 17 Apr 2020 10:40:51 -0700 Subject: [PATCH] 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 | 21 +++++++------- error_test.go | 2 +- oauth/client_test.go | 2 +- order/client_test.go | 2 +- stripe.go | 65 +++++++++++++++++++++++++++++--------------- stripe_test.go | 40 +++++++++++++-------------- testing/testing.go | 2 +- 7 files changed, 77 insertions(+), 57 deletions(-) diff --git a/README.md b/README.md index 01eb3584b7..64f9a65b3d 100644 --- a/README.md +++ b/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), } ``` diff --git a/error_test.go b/error_test.go index 06003035b7..010d02b5b1 100644 --- a/error_test.go +++ b/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) diff --git a/oauth/client_test.go b/oauth/client_test.go index dd98735fd7..ad3c804e7f 100644 --- a/oauth/client_test.go +++ b/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, }, ) diff --git a/order/client_test.go b/order/client_test.go index a22921bc98..89023aa5a8 100644 --- a/order/client_test.go +++ b/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} diff --git a/stripe.go b/stripe.go index 891a9f1810..39e831b0c5 100644 --- a/stripe.go +++ b/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,7 +156,7 @@ 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. @@ -160,8 +164,11 @@ 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, diff --git a/stripe_test.go b/stripe_test.go index 7f5ce9147b..71377f647d 100644 --- a/stripe_test.go +++ b/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) diff --git a/testing/testing.go b/testing/testing.go index 7522524137..a428df08a4 100644 --- a/testing/testing.go +++ b/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, },