349253 - implement backoff code for table updates

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

In the same vein as bug 349102, we want to degrade gracefully if url table updates are failing.  The logic is similar to what happens in the search service, if we get too many errors in a certain time period, we stop making requests for a while.  If we continue to get errors, we double our timeout.

Comment 1

[Tony Chang (Google)]

Attachment: v1: requestbackoff class and logic for table updates

There are some manual unittests at the end of the request-backoff.js.

Comment 2

[Niels Provos]

Comment on attachment 234518 [details] [diff] [review]
v1: requestbackoff class and logic for table updates

it looks like it takes max_errors + 1 errors to trigger the backoff.

i dont understand why you have a while loop.

you just want to shift so that you have at more MAX_ERRORS in the array.

you just need to check if the last MAX_ERRORS errors fall in the time period.

i am not sure if you want to clear the errors  if you get an okay response.  

rest looks good to me.

Comment 3

[Tony Chang (Google)]

Attachment: v2: requestbackoff class and logic for table updates

- use 'if' rather than 'while'
- make it more explicit what happens on successful request
- make sure to reset the object if data provider is changed

It triggers on max errors since it pushes the current time in the list first.  See unittests which confirm this.

Comment 4

[Niels Provos]

Comment on attachment 234613 [details] [diff] [review]
v2: requestbackoff class and logic for table updates

looks good.

Comment 5

[Darin Fisher]

Comment on attachment 234613 [details] [diff] [review]
v2: requestbackoff class and logic for table updates

>Index: toolkit/components/url-classifier/content/request-backoff.js

>+// HTTP responses that count as an error.
>+const HTTP_FOUND                 = 302; // DOS detected message
>+const HTTP_INTERNAL_SERVER_ERROR = 500;
>+const HTTP_BAD_GATEWAY           = 502;
>+const HTTP_SERVICE_UNAVAILABLE   = 503;

Why not restrict success codes instead?  A proxy server between Firefox
and the server might introduce other error codes besides these ones, so
perhaps it would be better to make this code less particular?


>+/**
>+ * Notify this object of the last server response.  If it's an error,
>+ */
>+RequestBackoff.prototype.noteServerResponse = function(status) {
>+  if (HTTP_INTERNAL_SERVER_ERROR == status ||
>+      HTTP_BAD_GATEWAY == status ||
>+      HTTP_SERVICE_UNAVAILABLE == status ||
>+      HTTP_FOUND == status) {

It seems like this should just test for non-200 responses or
maybe it should check for !200 and !206.


>Index: toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp

>+  rv = httpChannel->GetRequestSucceeded(&succeeded);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>   if (!succeeded) {
>+    // 404 or other error, pass error status back
>+    LOG(("HTTP request returned failure code."));
>+
>+    PRUint32 status;
>+    rv = httpChannel->GetResponseStatus(&status);
>+    NS_ENSURE_SUCCESS(rv, rv);

Note: connection failures that prevent a HTTP response from being generated
would not have a HTTP response code.  Instead GetResponseStatus would return
the error NS_ERROR_CONNECTION_REFUSED (or something like that).  Perhaps you
should treat those kinds of errors as reasons for backing off as well?


>+    nsCAutoString strStatus;
>+    strStatus.AppendInt(status);
>+    mErrorCallback->HandleEvent(strStatus);
>     return NS_ERROR_ABORT;

Shouldn't you generate an Error callback whenever you fail to generate the
Table callback?  In other words, maybe an Error callback should be generated
whenever you would need to return an error from this function.  Also, what
about the CancelStream case... should that result in an Error callback being
generated?

Comment 6

[Tony Chang (Google)]

(In reply to comment #5)
> 
> Why not restrict success codes instead?  A proxy server between Firefox
> and the server might introduce other error codes besides these ones, so
> perhaps it would be better to make this code less particular?
>
> It seems like this should just test for non-200 responses or
> maybe it should check for !200 and !206.

It seems like there should be three possibilities: ok, error with backoff, error without backoff.  Right now anything other than 200 would be error without backoff which seems ok.

The proxy case seems like something a bit fragile.  Would matching status in the range 500-505 be better?

> Note: connection failures that prevent a HTTP response from being generated
> would not have a HTTP response code.  Instead GetResponseStatus would return
> the error NS_ERROR_CONNECTION_REFUSED (or something like that).  Perhaps you
> should treat those kinds of errors as reasons for backing off as well?

Would this happen if the user wasn't online?  In which case, I don't think we want to trigger backoff.  Also, does onDataAvailable get fired in this case?

> Shouldn't you generate an Error callback whenever you fail to generate the
> Table callback?  In other words, maybe an Error callback should be generated
> whenever you would need to return an error from this function.  Also, what
> about the CancelStream case... should that result in an Error callback being
> generated?

I've renamed the method to be httpErrorCallback because that's what all we care about (I'll post the updated patch once I hear back about the other issues).

Comment 7

[Tony Chang (Google)]

Attachment: v3: check for connection refused and expand error codes

From discussion offline:
- error codes now include 302, 303, 307 and 5xx
- connection_refused also counts as an error

Comment 8

[Tony Chang (Google)]

on trunk

Comment 9

[Mike Schroepfer]

Comment on attachment 234965 [details] [diff] [review]
v3: check for connection refused and expand error codes

a=schrep/connor for drivers .

Comment 10

[Tony Chang (Google)]

on branch