From 9ff38e9093ef731554ff0f9afb0dff3384ecb936 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Thu, 14 Apr 2016 18:40:32 -0700 Subject: [PATCH 1/2] backoff: set default values on BackoffConfig Because most of the fields on `BackoffConfig` are unexported, correctly using the config requires copying from the default. This sets the defaults appropriately and falls back to a default if MaxDelay is negative or zero. Tests are added to ensure that the backoff is set correctly in common use cases. Signedroff-by: Stephen J Day Signed-off-by: Stephen J Day --- backoff.go | 9 +++++++++ backoff_test.go | 11 +++++++++++ clientconn.go | 12 ++++++++++++ clientconn_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+) create mode 100644 backoff_test.go diff --git a/backoff.go b/backoff.go index d0113ec9..1a42e1a6 100644 --- a/backoff.go +++ b/backoff.go @@ -46,6 +46,15 @@ type BackoffConfig struct { jitter float64 } +func (bc *BackoffConfig) setDefaults() { + md := bc.MaxDelay + *bc = *DefaultBackoffConfig + + if md > 0 { + bc.MaxDelay = md + } +} + func (bc *BackoffConfig) backoff(retries int) (t time.Duration) { if retries == 0 { return bc.baseDelay diff --git a/backoff_test.go b/backoff_test.go new file mode 100644 index 00000000..b3227b87 --- /dev/null +++ b/backoff_test.go @@ -0,0 +1,11 @@ +package grpc + +import "testing" + +func TestBackoffConfigDefaults(t *testing.T) { + b := BackoffConfig{} + b.setDefaults() + if b != *DefaultBackoffConfig { + t.Fatalf("expected BackoffConfig to pickup default parameters: %v != %v", b, *DefaultBackoffConfig) + } +} diff --git a/clientconn.go b/clientconn.go index 1562c0f9..ac56ea13 100644 --- a/clientconn.go +++ b/clientconn.go @@ -115,9 +115,21 @@ func WithPicker(p Picker) DialOption { } } +// WithBackoffMaxDelay configures the dialer to use the provided maximum delay +// when backing off after failed connection attempts. +func WithBackoffMaxDelay(md time.Duration) DialOption { + return WithBackoffConfig(&BackoffConfig{MaxDelay: md}) +} + // WithBackoffConfig configures the dialer to use the provided backoff // parameters after connection failures. +// +// Use WithBackoffMaxDelay until more parameters on BackoffConfig are opened up +// for use. func WithBackoffConfig(b *BackoffConfig) DialOption { + // Set defaults to ensure that provided BackoffConfig is valid and + // unexported fields get default values. + b.setDefaults() return withBackoff(b) } diff --git a/clientconn_test.go b/clientconn_test.go index 8eb1a225..88c3bb40 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -80,3 +80,42 @@ func TestCredentialsMisuse(t *testing.T) { t.Fatalf("Dial(_, _) = _, %v, want _, %v", err, ErrCredentialsMisuse) } } + +func TestWithBackoffConfigDefault(t *testing.T) { + testBackoffConfigSet(t, DefaultBackoffConfig) +} + +func TestWithBackoffConfig(t *testing.T) { + b := BackoffConfig{MaxDelay: DefaultBackoffConfig.MaxDelay / 2} + expected := b + expected.setDefaults() // defaults should be set + testBackoffConfigSet(t, &expected, WithBackoffConfig(&b)) +} + +func TestWithBackoffMaxDelay(t *testing.T) { + md := DefaultBackoffConfig.MaxDelay / 2 + expected := BackoffConfig{MaxDelay: md} + expected.setDefaults() + testBackoffConfigSet(t, &expected, WithBackoffMaxDelay(md)) +} + +func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOption) { + opts = append(opts, WithInsecure()) + conn, err := Dial("foo:80", opts...) + if err != nil { + t.Fatalf("unexpected error dialing connection: %v", err) + } + + if conn.dopts.bs == nil { + t.Fatalf("backoff config not set") + } + + actual, ok := conn.dopts.bs.(*BackoffConfig) + if !ok { + t.Fatalf("unexpected type of backoff config: %v", conn.dopts.bs) + } + + if *actual != *expected { + t.Fatalf("unexpected backoff config on connection: %v, want %v", actual, expected) + } +} From 8ef1dcabab9f28d439e9c7d23199cb27249cf09b Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Mon, 18 Apr 2016 11:33:39 -0700 Subject: [PATCH 2/2] backoff: make DefaultBackoffConfig a concrete value To enforce immutability of the `DefaultBackoffConfig`, we've made it a concrete value. While fields can still be set directly on the value, taking a copy will not incidentally pull a reference to the variable. Signed-off-by: Stephen J Day --- backoff.go | 11 +++++++---- backoff_test.go | 4 ++-- clientconn.go | 4 ++-- clientconn_test.go | 10 +++++----- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/backoff.go b/backoff.go index 1a42e1a6..36602caa 100644 --- a/backoff.go +++ b/backoff.go @@ -8,7 +8,7 @@ import ( // DefaultBackoffConfig uses values specified for backoff in // https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md. var ( - DefaultBackoffConfig = &BackoffConfig{ + DefaultBackoffConfig = BackoffConfig{ MaxDelay: 120 * time.Second, baseDelay: 1.0 * time.Second, factor: 1.6, @@ -33,7 +33,10 @@ type BackoffConfig struct { // MaxDelay is the upper bound of backoff delay. MaxDelay time.Duration - // TODO(stevvooe): The following fields are not exported, as allowing changes + // TODO(stevvooe): The following fields are not exported, as allowing + // changes would violate the current GRPC specification for backoff. If + // GRPC decides to allow more interesting backoff strategies, these fields + // may be opened up in the future. // baseDelay is the amount of time to wait before retrying after the first // failure. @@ -48,14 +51,14 @@ type BackoffConfig struct { func (bc *BackoffConfig) setDefaults() { md := bc.MaxDelay - *bc = *DefaultBackoffConfig + *bc = DefaultBackoffConfig if md > 0 { bc.MaxDelay = md } } -func (bc *BackoffConfig) backoff(retries int) (t time.Duration) { +func (bc BackoffConfig) backoff(retries int) (t time.Duration) { if retries == 0 { return bc.baseDelay } diff --git a/backoff_test.go b/backoff_test.go index b3227b87..847858c1 100644 --- a/backoff_test.go +++ b/backoff_test.go @@ -5,7 +5,7 @@ import "testing" func TestBackoffConfigDefaults(t *testing.T) { b := BackoffConfig{} b.setDefaults() - if b != *DefaultBackoffConfig { - t.Fatalf("expected BackoffConfig to pickup default parameters: %v != %v", b, *DefaultBackoffConfig) + if b != DefaultBackoffConfig { + t.Fatalf("expected BackoffConfig to pickup default parameters: %v != %v", b, DefaultBackoffConfig) } } diff --git a/clientconn.go b/clientconn.go index ac56ea13..9b29eabd 100644 --- a/clientconn.go +++ b/clientconn.go @@ -118,7 +118,7 @@ func WithPicker(p Picker) DialOption { // WithBackoffMaxDelay configures the dialer to use the provided maximum delay // when backing off after failed connection attempts. func WithBackoffMaxDelay(md time.Duration) DialOption { - return WithBackoffConfig(&BackoffConfig{MaxDelay: md}) + return WithBackoffConfig(BackoffConfig{MaxDelay: md}) } // WithBackoffConfig configures the dialer to use the provided backoff @@ -126,7 +126,7 @@ func WithBackoffMaxDelay(md time.Duration) DialOption { // // Use WithBackoffMaxDelay until more parameters on BackoffConfig are opened up // for use. -func WithBackoffConfig(b *BackoffConfig) DialOption { +func WithBackoffConfig(b BackoffConfig) DialOption { // Set defaults to ensure that provided BackoffConfig is valid and // unexported fields get default values. b.setDefaults() diff --git a/clientconn_test.go b/clientconn_test.go index 88c3bb40..336ad4dd 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -82,14 +82,14 @@ func TestCredentialsMisuse(t *testing.T) { } func TestWithBackoffConfigDefault(t *testing.T) { - testBackoffConfigSet(t, DefaultBackoffConfig) + testBackoffConfigSet(t, &DefaultBackoffConfig) } func TestWithBackoffConfig(t *testing.T) { b := BackoffConfig{MaxDelay: DefaultBackoffConfig.MaxDelay / 2} expected := b expected.setDefaults() // defaults should be set - testBackoffConfigSet(t, &expected, WithBackoffConfig(&b)) + testBackoffConfigSet(t, &expected, WithBackoffConfig(b)) } func TestWithBackoffMaxDelay(t *testing.T) { @@ -110,12 +110,12 @@ func testBackoffConfigSet(t *testing.T, expected *BackoffConfig, opts ...DialOpt t.Fatalf("backoff config not set") } - actual, ok := conn.dopts.bs.(*BackoffConfig) + actual, ok := conn.dopts.bs.(BackoffConfig) if !ok { - t.Fatalf("unexpected type of backoff config: %v", conn.dopts.bs) + t.Fatalf("unexpected type of backoff config: %#v", conn.dopts.bs) } - if *actual != *expected { + if actual != *expected { t.Fatalf("unexpected backoff config on connection: %v, want %v", actual, expected) } }