From 0da9b9bd3f69654da1bdef95f782836499c9f205 Mon Sep 17 00:00:00 2001 From: Alexis Charveriat Date: Sat, 16 Dec 2017 19:22:10 +0100 Subject: [PATCH 1/2] Enforced Google's specification regarding back-off mode As explained in the specs (https://developers.google.com/safe-browsing/v4/request-frequency), all non-successful return code shall trigger a back-off mode for the API. It was previously implemented for the threatListUpdates.fetch method, but not for fullHashes.find. For that purpose, back-off handling has been moved from the database scope to the API scope, providing a unified interface and behavior regardless of the used method. --- api.go | 37 ++++++++++++++++++++++++++++++++++--- database.go | 17 ++++------------- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/api.go b/api.go index 921f351..7f37400 100644 --- a/api.go +++ b/api.go @@ -17,17 +17,25 @@ package safebrowsing import ( "bytes" "context" + "errors" "fmt" "io/ioutil" + "math/rand" "net/http" "net/url" "strings" + "time" pb "github.com/google/safebrowsing/internal/safebrowsing_proto" "github.com/golang/protobuf/proto" ) +const ( + maxRetryDelay = 24 * time.Hour + baseRetryDelay = 15 * time.Minute +) + const ( findHashPath = "/v4/fullHashes:find" fetchUpdatePath = "/v4/threatListUpdates:fetch" @@ -41,8 +49,10 @@ type api interface { // netAPI is an api object that talks to the server over HTTP. type netAPI struct { - client *http.Client - url *url.URL + client *http.Client + url *url.URL + backOffMode bool + updateAPIErrors uint } // newNetAPI creates a new netAPI object pointed at the provided root URL. @@ -72,13 +82,17 @@ func newNetAPI(root string, key string, proxy string) (*netAPI, error) { q.Set("key", key) q.Set("alt", "proto") u.RawQuery = q.Encode() - return &netAPI{url: u, client: httpClient}, nil + return &netAPI{url: u, client: httpClient, backOffMode: false}, nil } // doRequests performs a POST to requestPath. It uses the marshaled form of req // as the request body payload, and automatically unmarshals the response body // payload as resp. func (a *netAPI) doRequest(ctx context.Context, requestPath string, req proto.Message, resp proto.Message) error { + if a.backOffMode { + return errors.New("API in back-off mode, refusing to execute") + } + p, err := proto.Marshal(req) if err != nil { return err @@ -95,8 +109,25 @@ func (a *netAPI) doRequest(ctx context.Context, requestPath string, req proto.Me } defer httpResp.Body.Close() if httpResp.StatusCode != 200 { + // Non 200 response, we shall enter back-off mode + a.backOffMode = true + go func(a *netAPI) { + // Backoff strategy: MIN((2**N-1 * 15 minutes) * (RAND + 1), 24 hours) + n := 1 << a.updateAPIErrors + delay := time.Duration(float64(n) * (rand.Float64() + 1) * float64(baseRetryDelay)) + if delay > maxRetryDelay { + delay = maxRetryDelay + } + a.updateAPIErrors++ + time.Sleep(delay) + a.backOffMode = false + }(a) + return fmt.Errorf("safebrowsing: unexpected server response code: %d", httpResp.StatusCode) } + + a.updateAPIErrors = 0 + body, err := ioutil.ReadAll(httpResp.Body) if err != nil { return err diff --git a/database.go b/database.go index 91bff38..399bccf 100644 --- a/database.go +++ b/database.go @@ -33,8 +33,6 @@ import ( // actually take. We add this time to the update period time to give some // leeway before declaring the database as stale. const ( - maxRetryDelay = 24 * time.Hour - baseRetryDelay = 15 * time.Minute jitter = 30 * time.Second ) @@ -72,7 +70,6 @@ type database struct { err error // Last error encountered readyCh chan struct{} // Used for waiting until not in an error state. last time.Time // Last time the threat list were synced - updateAPIErrors uint // Number of times we attempted to contact the api and failed log *log.Logger } @@ -221,18 +218,12 @@ func (db *database) Update(ctx context.Context, api api) (time.Duration, bool) { last := db.config.now() resp, err := api.ListUpdate(ctx, req) if err != nil { - db.log.Printf("ListUpdate failure (%d): %v", db.updateAPIErrors+1, err) + db.log.Printf("ListUpdate failure: %v", err) db.setError(err) - // backoff strategy: MIN((2**N-1 * 15 minutes) * (RAND + 1), 24 hours) - n := 1 << db.updateAPIErrors - delay := time.Duration(float64(n) * (rand.Float64() + 1) * float64(baseRetryDelay)) - if delay > maxRetryDelay { - delay = maxRetryDelay - } - db.updateAPIErrors++ - return delay, false + // Retry updating every minute in case of error. Error cases are handled + // by the API itself. + return 1 * time.Minute, false } - db.updateAPIErrors = 0 // add jitter to wait time to avoid all servers lining up nextUpdateWait := db.config.UpdatePeriod + time.Duration(rand.Int31n(60)-30)*time.Second From fcb77142b9a19facda173a86bd69e09746f4ec2a Mon Sep 17 00:00:00 2001 From: Alexis Charveriat Date: Mon, 18 Dec 2017 18:22:09 +0100 Subject: [PATCH 2/2] Fixed tests Since back-off is handled by the API, it doesn't make sense anymore to test it from the database. --- database_test.go | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-) diff --git a/database_test.go b/database_test.go index eb8e97b..b510239 100644 --- a/database_test.go +++ b/database_test.go @@ -552,38 +552,15 @@ func TestDatabaseUpdate(t *testing.T) { t.Fatalf("update 5, database state mismatch:\ngot %+v\nwant %+v", gotDB, wantDB) } - // Update 6: api is broken for some unknown reason. Checks the backoff + // Update 6: api is broken for some unknown reason. Checks retrial delay. errResponse = errors.New("Something broke") delay, updated = db.Update(context.Background(), mockAPI) if db.err == nil || updated { t.Fatalf("update 6, unexpected update success") } - minDelay := baseRetryDelay.Seconds() * float64(1) * float64(1) - maxDelay := baseRetryDelay.Seconds() * float64(2) * float64(1) - if delay.Seconds() < minDelay || delay.Seconds() > maxDelay { - t.Fatalf("update 6, Expected delay %v to be between %v and %v", delay.Seconds(), minDelay, maxDelay) - } - - // Update 7: api is still broken, check backoff is larger - delay, updated = db.Update(context.Background(), mockAPI) - if db.err == nil || updated { - t.Fatalf("update 7, unexpected update success") - } - minDelay = baseRetryDelay.Seconds() * float64(1) * float64(2) - maxDelay = baseRetryDelay.Seconds() * float64(2) * float64(2) - if delay.Seconds() < minDelay || delay.Seconds() > maxDelay { - t.Fatalf("update 7, Expected delay %v to be between %v and %v", delay.Seconds(), minDelay, maxDelay) - } - - // Update 8: api is still broken, check that backoff is larger than before - delay, updated = db.Update(context.Background(), mockAPI) - if db.err == nil || updated { - t.Fatalf("update 8, unexpected update success") - } - minDelay = baseRetryDelay.Seconds() * float64(1) * float64(4) - maxDelay = baseRetryDelay.Seconds() * float64(2) * float64(4) - if delay.Seconds() < minDelay || delay.Seconds() > maxDelay { - t.Fatalf("update 8, Expected delay %v to be between %v and %v", delay.Seconds(), minDelay, maxDelay) + expectedDelay = 1 * time.Minute + if delay != expectedDelay { + t.Fatalf("update 6, Expected delay %v to be %v", delay.Seconds(), expectedDelay.Seconds) } }