Merge lp:~stefanor/ibid/isitup-redirect-599410 into lp:~ibid-core/ibid/old-trunk-1.6

[Stefano Rivera]

Follow redirects in "is it up"

[lp:~stefanor/ibid/isitup-redirect-599410]

910. By Stefano Rivera on 2010-10-03

Actually give up waiting for a service to come up

[Jonathan Hitchcock (vhata)]

"More than max_hops" is not the same as "infinite redirect".

(And I don't think that "redirects" are the same as "hops".)

review: Needs Fixing

[Stefano Rivera (stefanor)]

> And I don't think that "redirects" are the same as "hops".

Agreed with you on that, didn't want to rename it in 0.1, I suppose I should split this into a separate branch for 0.2

> "More than max_hops" is not the same as "infinite redirect".

"Redirection limit reached"?

[Jonathan Hitchcock (vhata)]

> > "More than max_hops" is not the same as "infinite redirect".
>
> "Redirection limit reached"?

Works for me.

[lp:~stefanor/ibid/isitup-redirect-599410]

911. By Stefano Rivera on 2010-10-04

Response: infinite redircect loop -> Redirect limit reached

[Stefano Rivera (stefanor)]

r912

[lp:~stefanor/ibid/isitup-redirect-599410]

912. By Stefano Rivera on 2010-10-04

Rename max_hops to redirect_limit

[Jonathan Hitchcock (vhata)]

review: Approve

[Max Rabkin (max-rabkin)]

review: Approve

=== modified file 'ibid/plugins/network.py'
--- ibid/plugins/network.py	2010-04-15 15:18:16 +0000
+++ ibid/plugins/network.py	2010-10-04 17:41:21 +0000
@@ -229,8 +229,8 @@
     timeout = IntOption('timeout',
             u'Timeout for HTTP connections in seconds', 15)
     sites = DictOption('sites', u'Mapping of site names to domains', {})
-    max_hops = IntOption('max_hops',
-            u'Maximum hops in get/head when receiving a 30[12]', 3)
+    redirect_limit = IntOption('redirect_limit',
+            u'Maximum number of http redirects to follow', 5)
     whensitup_delay = IntOption('whensitup_delay',
             u'Initial delay between whensitup attempts in seconds', 60)
     whensitup_factor = FloatOption('whensitup_factor',
@@ -255,11 +255,10 @@
             reply = u'%s %s' % (status, reason)
 
             hops = 0
-            while status in (301, 302) and self._get_header(headers,
-                                                            'location'):
+            while 300 <= status < 400 and self._get_header(headers, 'location'):
                 location = self._get_header(headers, 'location')
                 status, reason, data, headers = self._request(location, 'GET')
-                if hops >= self.max_hops:
+                if hops >= self.redirect_limit:
                     reply += u' to %s' % location
                     break
                 hops += 1
@@ -301,7 +300,7 @@
                 url += '/'
         return url
 
-    def _isitup(self, url):
+    def _isitup(self, url, return_status=False, redirects=0):
         valid_url = self._makeurl(url)
         if Resolver is not None:
             r = Resolver()
@@ -309,29 +308,33 @@
             try:
                 r.query(host)
             except NoAnswer:
-                return False, u'No DNS A/CNAME-records for that domain'
+                return (False, valid_url,
+                        u'No DNS A/CNAME-records for that domain')
             except NXDOMAIN:
-                return False, u'No such domain'
+                return False, valid_url, u'No such domain'
 
         try:
             status, reason, data, headers = self._request(valid_url, 'HEAD')
-            # If the URL is only a hostname, we consider 400 series errors to
-            # mean up. Full URLs are checked for a sensible response code
-            if url == urlparse(url).path and '/' not in url:
-                up = status < 500
-            else:
-                up = status < 400
+            if 300 <= status < 400 and self._get_header(headers, 'location'):
+                if redirects > self.redirect_limit:
+                    return False, valid_url, u'Redirect limit reached'
+                return self._isitup(self._get_header(headers, 'location'),
+                                    return_status, redirects + 1)
+
+            up = status < 300
+            if return_status:
                 reason = u'%(status)d %(reason)s' % {
                     u'status': status,
                     u'reason': reason,
                 }
-            return up, reason
+            return up, valid_url, reason
         except HTTPException:
-            return False, u'Server is not responding'
+            return False, valid_url, u'Server is not responding'
 
     @match(r'^is\s+(\S+)\s+(up|down)$')
     def isit(self, event, url, type):
-        up, reason = self._isitup(url)
+        host_only = (url == urlparse(url).path and '/' not in url)
+        up, url, reason = self._isitup(url, return_status=not host_only)
         if up:
             if type.lower() == 'up':
                 event.addresponse(u'Yes, %s is up', url)
@@ -359,6 +362,7 @@
             event.addresponse(u"Sorry, it appears %s is never coming up. "
                               u"I'm not going to check any more.",
                               self._makeurl(url))
+            return
         delay *= self.whensitup_factor
         delay = max(delay, self.whensitup_maxdelay)
         ibid.dispatcher.call_later(delay, self._whensitup, event, url, delay,