On Tue, May 8, 2012 at 2:29 AM, Greg Stein <gst...@gmail.com> wrote:
> On May 7, 2012 8:16 PM, "Lieven Govaerts" <svn...@mobsol.be> wrote:
> >...
> > The problem is in ra_serf/util.c svn_ra_serf__handle_xml_parser:
> >
> > if (sl.code == 404 && ctx->ignore_errors == FALSE)
> > {
> > add_done_item(ctx);
> >
> > err = svn_ra_serf__handle_server_error(request, response, pool);
> >
> > SVN_ERR(svn_error_compose_create(
> > svn_ra_serf__handle_discard_body(request, response, NULL, pool),
> > err));
> >
> > When the response status of a PROPFIND request is 404, you see that the
> response body is discarded with calls to svn_ra_serf__handle_server_error
> and svn_ra_serf__handle_server_error.
> >
> > In your particular scenario, the status line of the response is already
> received, but the body is not. Reading from the response buckets returns
> EAGAIN status.
> > Problem: the add_done_item(ctx) line ensures that the request is
> considered as handled, while the response body is still waiting on the
> socket to be read. ra_serf will only run the serf loop again with the next
> request. If the connection is not closed directly, which here it isn't, the
> next request will have a response that doesn't match.
>
> Thanks for the excellent analysis of what Johan was running into.
>
> > The fix is to ensure that the request is only marked as handled when a.
> the response body has been discarded completely or a b. read error was
> encountered resulting in serf setting up a new connection. I don't have a
> tested solution, as my Windows vm was so nice to reboot to install some
> updates while I was in the middle of a debug session, and I don't have time
> now to start over.
>
> Not to worry. I've been working on exactly that stuff. In fact, the
> code you quoted is one of my targets to fix.
> svn_ra_serf__handle_server_error() is conceptually broken (and needs
> to be removed for the reason you state), as I noted in the log message
> of r1335217.
>
> My intent is to replace the code you quoted with something basically
> like: handler->server_error = alloc(). The core response handler will
> then start processing the body as an error.
>
So you mark the request as done as soon as the 404 is recognized, but then
add an extra reminder for ra_serf to continue processing the response until
read completely. That's better than what I suggested before.
There are a couple similar cases. I'm looking at them now to ensure
> the errors these things raise will propagate correctly, or to place
> the error creation elsewhere. It should be fixed within a few days
> (traveling tmw).
>
Nice. I'll keep my windows vm uptodate, I'll see if I can find some time
next week to test your changes.
Cheers,
> -g
>
Lieven
