Re: ra_serf crashes on Windows with AVG 2012 Surf-Shield

On Tue, May 8, 2012 at 2:29 AM, Greg Stein <gstein_at_gmail.com> wrote:
> On May 7, 2012 8:16 PM, "Lieven Govaerts" <svnlgo_at_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.

Thanks for looking into this, guys. Let me know if you'd like me to
run tests or something like that ...

Though this issue also hits 1.7, it doesn't seem like this is
something that can be backported easily (with all the recent changes
to ra_serf), does it? Unless a specific ("quick") fix is made for 1.7?
Anyway, I'm not going to push this. Serf is not the default in 1.7,
and the combination with AVG (Windows) is probably non existant at the
moment (there haven't been any user reports), so it's probably not
worth the effort.

-- 
Johan