Merge lp:~mbp/bzr/497274-http-405 into lp:bzr

This tries to handle 'http 405 not allowed' from Google code as meaning "not allowed to look for .bzr", ie there's no bzr branch there. That may allow foreign branches to work against it. However, their servers keep falling over with '503 not available' errors, which make testing it difficult.

I've only tested this interactively not added tests. I probably should.

This prints and logs the actual message from the http response page.

It also tries to unify http response handling currently scattered across _pycurl.py into one method in the base class. This may be slightly dangerous to change as it's lightly tested and tries to work around server quirks, but any breakage should be shallow and it's worth clearing it up I think. I haven't yet gone through the urllib implementation.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/497274-http-405 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #497274 should interpret http 405 "not allowed" as "no smart server here" - breaks foreign branches on google code
> https://bugs.launchpad.net/bugs/497274
>
>
> This tries to handle 'http 405 not allowed' from Google code as meaning "not allowed to look for .bzr", ie there's no bzr branch there. That may allow foreign branches to work against it. However, their servers keep falling over with '503 not available' errors, which make testing it difficult.
>
> I've only tested this interactively not added tests. I probably should.
>
> This prints and logs the actual message from the http response page.
>
> It also tries to unify http response handling currently scattered across _pycurl.py into one method in the base class. This may be slightly dangerous to change as it's lightly tested and tries to work around server quirks, but any breakage should be shallow and it's worth clearing it up I think. I haven't yet gone through the urllib implementation.
>

I would ask Vincent for some help, but as I understand he already has
all the infrastructure to create an HTTP server that gives canned
responses. So setting one up to return 405 on .bzr/branch-format and
ensuring that bzr then raises NoSuchBranch rather than
InvalidHTTPResponse seems a reasonable high-level test.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkspBNQACgkQJdeBCYSNAAMOsACguGOLfNw3cNBoJNFYjSMjl3io
biAAn3asxZvkOtMob2ryoGSEF7laoBGT
=JW9R
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/497274-http-405 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #497274 should interpret http 405 "not allowed" as "no smart server here" - breaks foreign branches on google code
> https://bugs.launchpad.net/bugs/497274
>
>
> This tries to handle 'http 405 not allowed' from Google code as meaning "not allowed to look for .bzr", ie there's no bzr branch there. That may allow foreign branches to work against it. However, their servers keep falling over with '503 not available' errors, which make testing it difficult.
>
> I've only tested this interactively not added tests. I probably should.
>
> This prints and logs the actual message from the http response page.
>
> It also tries to unify http response handling currently scattered across _pycurl.py into one method in the base class. This may be slightly dangerous to change as it's lightly tested and tries to work around server quirks, but any breakage should be shallow and it's worth clearing it up I think. I haven't yet gone through the urllib implementation.
>

Oh, and:

 review: approve

- From me, though it would be nice to have the test case before landing.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkspBRcACgkQJdeBCYSNAAPp+ACfQl/BMB8fhepPk1/O5vH2FP5v
gp8AoJXZrwqN8ebjB9aAi36oc11xabJz
=mBqz
-----END PGP SIGNATURE-----

> This tries to handle 'http 405 not allowed' from Google code as meaning "not
> allowed to look for .bzr", ie there's no bzr branch there. That may allow
> foreign branches to work against it. However, their servers keep falling over
> with '503 not available' errors, which make testing it difficult.

Then look at lp:~vila/bzr/497274-http-405 for an alternative, minimal (and probably incomplete)
fix with tests.

_post() is used only by smart_http_request() and the later already translate
InvalidHttpResponse into SmartProtocolError which higher layers interpret as NotABranch when needed.

>
> I've only tested this interactively not added tests. I probably should.

See above.

>
> This prints and logs the actual message from the http response page.

I doubt users are really interested in the general case, muttering may be enough though.

>
> It also tries to unify http response handling currently scattered across
> _pycurl.py into one method in the base class.

I'm pretty sure I went mostly in the opposite direction in the past based on problems
encountered while implementing the webdav support.

I didn't try to unify both implementations either... mostly because we will deprecate pycurl
one day and I was happy with the urllib one. The idea there is that there are three categories
of error codes:
- the ones that represent success,
- the ones that represent known failures,
- the others

Upon reception, the callers must handle the success cases, may handle some known
failures specifically before relying on a fallback for the remaining ones.

Basically the http requests are generic enough that the same error code can be interpreted
differently in different contexts. Unifying the error handling too much makes it harder
to raise the correct bzr exceptions.

So far, only 403 was really unambiguously common, even 404 needed to be handled separately
for HEAD (which is the main reason why I prefer to *not* have it in the common code).

All in all, that gives a coherent way to code all the methods with limited duplication
(mostly 404 but everybody knows what 404 means :)

> This may be slightly dangerous
> to change as it's lightly tested and tries to work around server quirks, but
> any breakage should be shallow and it's worth clearing it up I think. I
> haven't yet gone through the urllib implementation.

See above, that's a one-line fix, the pycurl implementation doesn't even need to be fixed.

That may explains why you can't test it against google, I suspect others use the
urllib implementation and needs the fix.

Note that I fixed a bug in send_http_smart_request so you want to backport at least that part.

So I vote needs_fixing and will mark as Work In Progress.

> > This tries to handle 'http 405 not allowed' from Google code as meaning "not
> > allowed to look for .bzr", ie there's no bzr branch there. That may allow
> > foreign branches to work against it. However, their servers keep falling
> over
> > with '503 not available' errors, which make testing it difficult.
>
> Then look at lp:~vila/bzr/497274-http-405 for an alternative, minimal (and
> probably incomplete)
> fix with tests.

I'll merge that into mine and see how it looks.

> _post() is used only by smart_http_request() and the later already translate
> InvalidHttpResponse into SmartProtocolError which higher layers interpret as
> NotABranch when needed.
>
> >
> > I've only tested this interactively not added tests. I probably should.
>
> See above.
>
> >
> > This prints and logs the actual message from the http response page.
>
> I doubt users are really interested in the general case, muttering may be
> enough though.

They might not be, but only giving the status code is not very helpful if there is something mysterious going on, like the server being overloaded. We tend to get bug reports about this - though just getting it into the trace should help us with that.

>
> >
> > It also tries to unify http response handling currently scattered across
> > _pycurl.py into one method in the base class.
>
> I'm pretty sure I went mostly in the opposite direction in the past based on
> problems
> encountered while implementing the webdav support.
>
> I didn't try to unify both implementations either... mostly because we will
> deprecate pycurl
> one day and I was happy with the urllib one. The idea there is that there are
> three categories
> of error codes:
> - the ones that represent success,
> - the ones that represent known failures,
> - the others
>
> Upon reception, the callers must handle the success cases, may handle some
> known
> failures specifically before relying on a fallback for the remaining ones.
>
> Basically the http requests are generic enough that the same error code can be
> interpreted
> differently in different contexts. Unifying the error handling too much makes
> it harder
> to raise the correct bzr exceptions.
>
> So far, only 403 was really unambiguously common, even 404 needed to be
> handled separately
> for HEAD (which is the main reason why I prefer to *not* have it in the common
> code).
>
> All in all, that gives a coherent way to code all the methods with limited
> duplication
> (mostly 404 but everybody knows what 404 means :)

There are two types of possible duplication: across different call paths (get vs post vs others) and across different http client implementations. I can see how different callers might want different interpretations for some codes, but I don't see why we would want different interpretations across urllib and pycurl, unless it really is something where the client library interferes with the result, as it may for redirects.

>
> > This may be slightly dangerous
> > to change as it's lightly tested and tries to work around server quirks, but
> > any breakage should be shallow and it's worth clearing it up I think. I
> > haven't yet gone through the urllib implementatio...

@vila: How do you see the interaction between the various http exception classes working. For instance, why is this code as it is:

    def http_error_default(self, req, fp, code, msg, hdrs):
        if code == 403:
            raise errors.TransportError(
                'Server refuses to fulfill the request (403 Forbidden)'
                ' for %s' % req.get_full_url())
        else:
            raise errors.InvalidHttpResponse(req.get_full_url(),
                                             'Unable to handle http code %d: %s'
                                             % (code, msg))

google code seems to be up again and I've just been testing this interactively, and I can't actually get trunk to fail there, even with pycurl disabled. So I'm not sure what the future of this patch is: to me at least some of it seems like a useful cleanup, but given all the quirks here I don't think it's worth landing it without at least one real-world test showing it's better.

vila, how did you reproduce an improvement with your patch?

I've split out the html error handling (pycurl only) into https://code.edge.launchpad.net/~mbp/bzr/http-messages

>>>>> "martin" == Martin Pool <email address hidden> writes:

Disclaimer: I wrote my patch in urgency without trying to
integrate it with yours, as such I tried to address the missing
parts. The were no tests and no urllib implementation. I stopped
there and didn't explain it as well as I should have, see below
for more comments.

<snip/>

    martin> I'll merge that into mine and see how it looks.

That was the idea, I'm fine with whatever end result you come up
with.

<snip/>

    >> I doubt users are really interested in the general case,
    >> muttering may be enough though.

    martin> They might not be, but only giving the status code is
    martin> not very helpful if there is something mysterious
    martin> going on, like the server being overloaded. We tend
    martin> to get bug reports about this - though just getting
    martin> it into the trace should help us with that.

Yup. I was a bit uncomfortable with unhtml() but not that
much. My main concern is that its a bit brute force and in
presence of errors I like, as a dev, to have access to the raw
data. Users have others expectancies and I don't a clear answer
about what should be presented.

<snip/>

    martin> There are two types of possible duplication: across
    martin> different call paths (get vs post vs others) and
    martin> across different http client implementations. I can
    martin> see how different callers might want different
    martin> interpretations for some codes, but I don't see why
    martin> we would want different interpretations across urllib
    martin> and pycurl, unless it really is something where the
    martin> client library interferes with the result, as it may
    martin> for redirects.

So, thanks for bringing a fresh eye here. Historically I've never
attempt to unify the two implementations very hard based on the
assumption that pycurl will be deprecated (shudder).

But as you point above, both goals are a bit contradictory, you
have to care to leave errors escape the lower layer handling so
that the higher (common) one can work.

<snip/>

    martin> So your code has, in handle_response:

    martin> elif code == 405:
    martin> # The server refused to handle the request, the data presumably
    martin> # contains details about the error and not useful data, but we leave
    martin> # that to the higher levels.
    martin> return data

    martin> which strongly looks like you're going to treat the
    martin> html text of the error response as a valid response
    martin> body, which is definitely not right. OK, apparently
    martin> this doesn't necessarily happen because the base
    martin> class send_http_smart_request translates it, but
    martin> there's still some risk of that.

Yup, that's why I said the implementation is incomplete. This was
the less intrusive fix but I was not happy with the risk.

    martin> If you pass around (http_status, body) too much then
    martin> multiple layers need to know how to interpret the
    martin> status code. Without understanding it, they can't
    martin> understand how to use the body data. For insta2nce,
    martin> send_http_smart_request ...

    martin> I've split out the html error handling (pycurl only) into
    martin> https://code.edge.launchpad.net/~mbp/bzr/http-messages

The diff isn't online yet, I'll give it a look in a couple of
days. See my other mail and feel free to bring more of your work
based on my remarks there.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> martin> I guess I don't properly understand whether you think
> martin> my patch is actually wrong or just needs tests. I
> martin> will try to use your tests with mine and see where we
> martin> get to.
>
> I hope I clarified that: except for 404, I'm fine with a common
> error handling, may be I just haven't seen yet a good way to
> implement that but I didn't spend much time on it either and I
> trust you to find it ;)
>

If the only code that uses 404 specially is HEAD, then I would be happy
to have it trap the exception. #1 because I don't think we use
"transport.has*" in any live code path. LBYL vs EAFTP.

I think the only code that uses it is probably old-format repositories.
(Where having a revision was indicated by the presence of the
'.bzr/revisions/$REVID' file.)

I'm pretty sure everything else would treate 404 as NoSuchFile.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAksvi9wACgkQJdeBCYSNAAM7ygCffCBwDol9uBmR8XgrlxnQTe1e
vOMAoKCYRxzzy7RQmM+Byq7iKO7EP0VF
=dmsP
-----END PGP SIGNATURE-----

2009/12/22 John A Meinel <email address hidden>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
>>     martin> I guess I don't properly understand whether you think
>>     martin> my patch is actually wrong or just needs tests.  I
>>     martin> will try to use your tests with mine and see where we
>>     martin> get to.
>>
>> I hope I clarified that: except for 404, I'm fine with a common
>> error handling, may be I just haven't seen yet a good way to
>> implement that but I didn't spend much time on it either and I
>> trust you to find it ;)
>>
>
> If the only code that uses 404 specially is HEAD, then I would be happy
> to have it trap the exception. #1 because I don't think we use
> "transport.has*" in any live code path. LBYL vs EAFTP.
>
> I think the only code that uses it is probably old-format repositories.
> (Where having a revision was indicated by the presence of the
> '.bzr/revisions/$REVID' file.)
>
> I'm pretty sure everything else would treate 404 as NoSuchFile.

I think it would be reasonable for .has() to just catch NoSuchFile.

Having looked at this a bit more: InvalidHttpResponse mixes both "not
valid http" and "not the http code we expected." So perhaps a good
step would be to separate that into "HttpError" containing all the
right fields, then we can translate that into a specific bzr error,
taking into account the type of operation we were doing.

However, since imports from google are apparently now working, this is
not urgent for me.

--
Martin <http://launchpad.net/~mbp/>

@poolie: From the comments, I get the impression that you have un-pushed changes there. If that's right, can you update this mp ? I'm pretty sure it's in a good enough state to land on trunk if only to allow whoever being able to reproduce the issue to test it.

This probably needs to be refreshed against the current trunk, watch the conflicts with an hawk eye ;)

vila, on the whole, I'll just abandon this branch. I think the situation it was trying to fix may have been a transient problem at Google's end, and there's no longer enough obvious benefit to be worth changing it.

>>>>> Martin Pool <email address hidden> writes:

    > vila, on the whole, I'll just abandon this branch. I think the
    > situation it was trying to fix may have been a transient problem
    > at Google's end, and there's no longer enough obvious benefit to
    > be worth changing it.

Did you write this before or after commenting on bug #497274 ?

https://bugs.launchpad.net/bzr/+bug/497274/comments/15 kind of imply you
still consider this proposal to be useful no ?

On 21 February 2011 18:48, Vincent Ladeuil <email address hidden> wrote:
>>>>>> Martin Pool <email address hidden> writes:
>
>    > vila, on the whole, I'll just abandon this branch.  I think the
>    > situation it was trying to fix may have been a transient problem
>    > at Google's end, and there's no longer enough obvious benefit to
>    > be worth changing it.
>
> Did you write this before or after commenting on bug #497274 ?
>
> https://bugs.launchpad.net/bzr/+bug/497274/comments/15 kind of imply you
> still consider this proposal to be useful no ?

I wrote that before.

I don't think this mp is still in progress, but it can be resurrected
if that's useful.