On Tue, Jun 12, 2012 at 2:18 PM, Lieven Govaerts <l...@mobsol.be> wrote:
> On Tue, Jun 12, 2012 at 2:00 PM, Justin Erenkrantz
> <jus...@erenkrantz.com> wrote:
>> On Tue, Jun 12, 2012 at 1:56 PM, Lieven Govaerts <l...@mobsol.be> wrote:
>>>> How is it supposed to retry? ra_serf needs to let the context_run
>>>> loop execute again to pull off the remaining bits from the socket.
>>>> Without the patch, we treat EAGAIN as a fatal error and it gets
>>>> returned all the way to the client. -- justin
>>>
>>> Hm, I see now, it's not the return code of the response handler here,
>>> but the apr error wrapped in the svn error object.
>>
>> Right.
>>
>>> So why would we insist on treating EAGAIN as error in the response
>>> handler? There doesn't seem to be any added value of passing this
>>> expected return code as error all the way back to the look.
>>
>> So, what should we do when EAGAIN is returned by the socket? The code
>> needs to know to run the loop again...without triggering a fatal
>> error.
>
> The loop will continue automatically of the status returned from
> serf_context_run = APR_SUCCESS and if there is no err object set by
> the response handler.
>
> So the response handler should not set an err object, but return
> APR_EAGAIN as return code to serf.
> Hm, this is what already happens in the process_body: section of
> handle_response in ra_serf/util.c.
>
> When does EAGAIN get returned? When reading the status lines, response
> headers, body, or error?
>
Attached patch shows what I suggest. The case you probably encounter
is when the response is handled by handle_server_error.
This is untested, I don't have a Windows setup ready and didn't
install my build tools yet after upgrade to OS X S.L.
>>
>> I've never felt we got the error handling quite right in
>> serf...*headbang* -- justin
>
> Lieven
Index: subversion/libsvn_ra_serf/util.c
===================================================================
--- subversion/libsvn_ra_serf/util.c (revision 1349292)
+++ subversion/libsvn_ra_serf/util.c (working copy)
@@ -1847,6 +1847,8 @@
### packet. we need ongoing parsing. see SERVER_ERROR down below
### in the process_body: area. we'll eventually move to that. */
SVN_ERR(handle_server_error(request, response, scratch_pool));
+ if (err)
+ goto handle_error;
if (!handler->session->pending_error)
{
@@ -1926,6 +1928,7 @@
handler->response_baton,
scratch_pool);
+handle_error:
if (err
&& (!SERF_BUCKET_READ_ERROR(err->apr_err)
|| APR_STATUS_IS_ECONNRESET(err->apr_err)))
