HttpTransport_urllib.has() tries to access URLError.code, causing an AttributeError

Bug #42407 reported by Guilherme Salgado
2
Affects Status Importance Assigned to Milestone
Bazaar
Fix Released
Medium
Vincent Ladeuil

Bug Description

That method catches an URLError and then tries to access its 'code' attribute, but that class doesn't have this attribute like HTTPError.

Revision history for this message
Colin Watson (cjwatson) wrote :

I also saw this today with bzr 0.8.

Changed in bzr:
status: Unconfirmed → Confirmed
Revision history for this message
John A Meinel (jameinel) wrote :

My guess is that this is line 118 of bzrlib/transport/http/_urllib.py, where we have:
       except urllib2.URLError, e:
            mutter('url error code: %s for has url: %r', e.code, abspath)
            if e.code == 404:
                return False
            raise

I do believe this is a bug, but I would like to know how to make it happen, so that we can write a test for it, and fix it properly.

Can you describe what you were doing when this happened.

Revision history for this message
Guilherme Salgado (salgado) wrote :

It happened during the run of the launchpad script that's responsible for mirroring branches, but I don't know what's the mirror location nor how to reproduce it. All I have is the traceback:

...
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/canonical/launchpad/scripts/supermirror/branchtomirror.py", line 42, in mirror
    dest_branch.pull(srcbranch, overwrite=True)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/decorators.py", line 51, in write_locked
    return unbound(self, *args, **kwargs)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/branch.py", line 1020, in pull
    self.update_revisions(source,stop_revision)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/branch.py", line 976, in update_revisions
    self.fetch(other, stop_revision)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/decorators.py", line 51, in write_locked
    return unbound(self, *args, **kwargs)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/branch.py", line 216, in fetch
    pb=pb)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/repository.py", line 143, in fetch
    pb=pb)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/decorators.py", line 51, in write_locked
    return unbound(self, *args, **kwargs)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/repository.py", line 1192, in fetch
    pb=pb)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/fetch.py", line 103, in __init__
    self.__fetch()
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/fetch.py", line 127, in __fetch
    self._fetch_revision_texts(revs)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/fetch.py", line 152, in _fetch_revision_texts
    pb=self.pb)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/store/__init__.py", line 114, in copy_multi
    self._copy_one(fileid, None, other, pb)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/store/text.py", line 93, in _copy_one
    path = other._get_name(fileid, suffix)
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/store/__init__.py", line 203, in _get_name
    if self._transport.has(name):
  File "/srv/sm-ng/production/launchpad/cronscripts/../lib/bzrlib/transport/http.py", line 182, in has
    mutter('url error code: %s for has url: %r', e.code, path)
AttributeError: URLError instance has no attribute 'code'

Revision history for this message
Michael Ellerman (michael-ellerman) wrote :

URLError doesn't have a code attribute, its subclass HTTPError does. So I think this should probably be split into two except clauses, one for each type.

For a test, this should do it:

>>> try: urllib2.urlopen('http:///foo')
... except urllib2.URLError, e:
... print e.__dict__
...
{'reason': 'no host given', 'args': ('no host given',)}
>>>

Revision history for this message
John A Meinel (jameinel) wrote :

The only way I've been able to reproduce this is by using an HTTP url which does not reference an existing server.
So:

t = bzrlib.transport.get_transport('http+urllib://foobaz.launchpad.net/')

t.has('anything')

This will raise a plain URLError exception.
The only info I can find is that e.reason or e.args[0] is a socket.error, and the socket.error .args[0] is an error code that can be looked up in socket.E* (it happens to be socket.EAI_NONAME == -2).

What I really don't understand is why you aren't getting this error until deep into the file copying. It also would seem that this is an older format branch, since it is using a 'text_store'. (though revisions in v6 branches were a TextStore.)
The code path seems to be tweaked when a remote store is TextStore and the local branch is not (so you are pulling from a v6 branch into a metadir branch).

We do indeed have a bug in our 'has()' code. Mostly we avoid using Transport.has() (there are bugs in the pycurl version as well, it raises NoSuchFile instead of returning False).

I could fix it so that 'has()' swallows the error. But I think what you really want to get is a ConnectionError.

Revision history for this message
John A Meinel (jameinel) wrote :

Now we at least don't get AttributeError (we might get URLError, though)

Changed in bzr:
assignee: nobody → v-ladeuil
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.