Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforced Google's specification regarding back-off mode #78

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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.
3kt committed Dec 18, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 0da9b9bd3f69654da1bdef95f782836499c9f205
37 changes: 34 additions & 3 deletions api.go
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lock is needed for this. The 'database' was handling locking before, but now we have HashLookup requests happening too (which can happen concurrently).

}

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a sleeping goroutine, how about we have an error count, and a 'time we can make requests again', along with a RWMutex.

So at the top, it would just do:

a.mu.RLock()
if time.Now().Before(someRetryTime) {
  return someError
}
a.mu.RUnlock()

then later:

if httpResp.StatusCode != 200 {
  return func() error {
    r.mu.Lock()
    defer r.mu.Unlock()
    if time.Now().Before(someRetryTime) {
      // Another request already recorded an error in the time we made the request
     return someError
    }
    // Calculate backoff time, update consecutive errors
    return someError
  }()
}

// Everything good!
mu.Lock()
a.updateAPIErrors = 0
// optionally zero someRetryTime.
mu.Unlock()

Obviously doesn't have to be exactly like this. This way makes it possible to export the 'time we can make requests again', whether via an error, or more directly through a field.

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
17 changes: 4 additions & 13 deletions database.go
Original file line number Diff line number Diff line change
@@ -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