471192 - HTTP error codes are not recognized properly

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Luca Tettamanti]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.0.3) Gecko/2008092816 Iceweasel/3.0.4 (Debian-3.0.4-1)
Build Identifier: hg 1271:e1c4b9fcbe27

Resource class checks a few HTTP status codes to determine whether the request was successful or not.
HTTP 5xx error codes (see RFC2616) are all server-side errors, but weave - with the exception of 501 (not implemented) and 505 (HTTP version not supported) - consider them as "success"; this is bad since the server component uses 503 (service unavailable) to signal error conditions (mostly database outages).

Reproducible: Always

Steps to Reproduce:
1. Stop the DB backend of the server component to simulate the error condition
2. Try and login
Actual Results:  
From weave log:

Net.Resource	DEBUG	GET request for http://127.0.0.1/~kronos/0.3/user/kronos
Net.Resource	DEBUG	GET request successful (503)


Expected Results:  
From weave log, with proposed patch applied:

Net.Resource	DEBUG	GET request for http://127.0.0.1/~kronos/0.3/user/kronos
Net.Resource	DEBUG	GET request failed (503)
Net.Resource	DEBUG	Error response: 
"Database unavailable"

Comment 1

[Luca Tettamanti]

Attachment: Add all 5xx HTTP error codes to the "failure" list.

Comment 2

[Dan Mills [:thunder]]

Indeed, we don't handle 503s correctly.  Weave should check for a Retry-After header and retry after that time, otherwise treat a 503 as a 500 (per the spec).  But 500 itself is not handled correctly either.

The reason we handle 501 and 505 correctly but not the rest is that those are the ones in the spec that are 100% guaranteed to be permanent errors (retrying will not help) -- at least when I took a quick glance at the spec at the time.

Comment 3

[Luca Tettamanti]

AFAIK Retry-After is optional and in my experience very uncommon, so I'm not sure it's worth adding a special case for 503.
Anyway I'd put the retry logic at an higher level, since the desired behaviour is context-specific: during scheduled sync the best option is probably "don't care, retry at the next scheduled sync", while at login time it may make sense to retry a few times.

Comment 4

[Dan Mills [:thunder]]

503 + Retry-After is uncommon in the wild, but it's a good way for the server to tell clients to back off when it's under heavy load.  We have our own server, so we might use that (we expect Weave servers to be under high load).

You may be right anyway and we could just fail and let the next sync take care of it. Hmm.

Comment 7

[Dan Mills [:thunder]]

Punting to 0.4, we need to look at this code again and figure out where things stand.

Comment 8

[Mike Connor [:mconnor]]

We should get a plan on this sooner or later, including the bug on supporting backoff properly.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 354495 [details] [diff] [review]
Add all 5xx HTTP error codes to the "failure" list.

Pretty sure this isn't the right fix, from discussion in triage.

Comment 10

[Mike Connor [:mconnor]]

Fixed by bug 497938