Skip to content

Commit

Permalink
[FAB-4743] Harden backoff policy code in deliverClient
Browse files Browse the repository at this point in the history
The delivery client has a backoff policy that specifies
an amount of time to sleep and whether to continue to retry
 based on an attempt number.

Currently the code that calculates the amount of time to sleep
is prone to overflow to a negative number.

However, the fact that the code only retries a limited number
of times (not indefinitely) saves us from the overflow.

I would like to make the code that calculates the time duration
to sleep to never overflow in order for if someone in the future
to change the attempt threshold and forget about the overflow problem
lurking around the corner, we won't have a problem.

Change-Id: Ie0d365e879eb71141c278231819e52e341b73fe5
Signed-off-by: Yacov Manevich <yacovm@il.ibm.com>
  • Loading branch information
yacovm committed Jun 15, 2017
1 parent 4dc0370 commit bc6db92
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 1 deletion.
5 changes: 4 additions & 1 deletion core/deliverservice/deliveryclient.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ func init() {
var (
reConnectTotalTimeThreshold = time.Second * 60 * 5
connTimeout = time.Second * 3
reConnectBackoffThreshold = float64(time.Hour)
)

// DeliverService used to communicate with orderers to obtain
Expand Down Expand Up @@ -181,7 +182,9 @@ func (d *deliverServiceImpl) newClient(chainID string, ledgerInfoProvider blocks
if elapsedTime.Nanoseconds() > reConnectTotalTimeThreshold.Nanoseconds() {
return 0, false
}
return time.Duration(math.Pow(2, float64(attemptNum))) * time.Millisecond * 500, true
sleepIncrement := float64(time.Millisecond * 500)
attempt := float64(attemptNum)
return time.Duration(math.Min(math.Pow(2, attempt)*sleepIncrement, reConnectBackoffThreshold)), true
}
connProd := comm.NewConnectionProducer(d.conf.ConnFactory(chainID), d.conf.Endpoints)
bClient := NewBroadcastClient(connProd, d.conf.ABCFactory, broadcastSetup, backoffPolicy)
Expand Down
15 changes: 15 additions & 0 deletions core/deliverservice/deliveryclient_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package deliverclient

import (
"errors"
"runtime"
"sync"
"sync/atomic"
Expand Down Expand Up @@ -406,6 +407,20 @@ func TestDeliverServiceBadConfig(t *testing.T) {
assert.Nil(t, service)
}

func TestRetryPolicyOverflow(t *testing.T) {
connFactory := func(channelID string) func(endpoint string) (*grpc.ClientConn, error) {
return func(_ string) (*grpc.ClientConn, error) {
return nil, errors.New("")
}
}
client := (&deliverServiceImpl{conf: &Config{ConnFactory: connFactory}}).newClient("TEST", &mocks.MockLedgerInfo{Height: uint64(100)})
assert.NotNil(t, client.shouldRetry)
for i := 0; i < 100; i++ {
retryTime, _ := client.shouldRetry(i, time.Second)
assert.True(t, retryTime <= time.Hour && retryTime > 0)
}
}

func assertBlockDissemination(expectedSeq uint64, ch chan uint64, t *testing.T) {
select {
case seq := <-ch:
Expand Down

0 comments on commit bc6db92

Please sign in to comment.