[MERGE] Fix bug #225020 by catching the CURLE_SEND_ERROR error more broadly.

[Vincent Ladeuil]

>>>>> "john" == John Arbash Meinel <john at arbash-meinel.com> writes:

    john> Vincent Ladeuil wrote:
    >> So that bug want to play more...
    >> 
    >> The previous fix was too restrictive.
    >> 
    >> This one catch additional cases and should be final.
    >> 
    >> I filed a bug upstream against libcurl for which a patch has been
    >> proposed.
    >> 
    >> I tested the upstream patch against bzr.dev without my previous
    >> patch and with this patch. This upstream patch fixed the bug.
    >> 
    >> Yet, this patch implements a workaround that fix our problem
    >> whether or not the upstream patch is applied.
    >> 
    >> To summarize:
    >> 
    >> - bzr without any patch, libcurl-7.18.0 without patches: 23 tests failing,

    john> Is that without any patch, or without your *earlier* (403) patch?

Without any. I wanted to get a "clean" start.

    john> I wonder if we should at least check that we actually
    john> got a 4XX error, rather than assuming so.

I went there for the first patch :-/

And I now think it was *the* error. I did make the test suite pass
by replacing (in my first proposal)
  'if code == 403'
 by
  'if code in (403, 501)'

Note that 404 (for example) is neither mandatory nor
recommended. Same thing for 401.

And then... I thought that I was trying too hard :-)

And more seriously that I failed to correctly fix the bug by
trying to isolate it more precisely at a level which doesn't give
enough infos to do that.

So I went with *this* patch, considering that the rare
occurrences will stay rare but adding a 'mutter' call in case we
encounter it again.

But since it doesn't happen for 404 nor 401 which ones should I
had ? I'm afraid I will introduce more false fixes that way.

The fact is this same bug has already masked other problems
because it made it impossible to see the real error (I don't
remember the details, but in once case a squid proxy were acting
and all we got was that 55 select/poll error).

Simply swallowing that error, knowing that it's precisely the bug
symptom, is, IMO, the best fix, because the callers will act
according to the error code. And I should really say 'caller'
here since only the smart server is issuing POST requests and any
code different from 200 is a SmartProtocolError.

And last nail in the coffin, if we get a 200, it means we
successfully send the body and the bug occurs *while* trying to
send the body.

    john> For example, what if we got a 200 OK, but the
    john> connection died (network went down).

Trapped elsewhere as most other expected errors.

This bug is really in curl and happens only on errors, 200 is not
concerned, if the network goes down, that will be seen
differently.

    john> Anyway, I'm okay with this patch, but:

    john> BB:tweak

Are my explanations sufficient to promote that to approve ?

If not what do you recommand ?

   Vincent