Re: svn commit: r36913 - trunk/subversion/libsvn_ra_serf

I'd go one better and say this might be appropriate for 1.5.x, too.

On Tue, Mar 31, 2009 at 23:25, Ben Collins-Sussman <sussman_at_red-bean.com> wrote:
> Definitely worth a 1.6.1 backport, I think.  ra_serf currently fails
> to commit against googlecode.com due to some google-specific
> infrastructure latency in responses, which then triggers this infinite
> loop in ra_serf.
>
> On Tue, Mar 31, 2009 at 4:22 PM, Ben Collins-Sussman
> <sussman_at_red-bean.com> wrote:
>> Author: sussman
>> Date: Tue Mar 31 14:22:56 2009
>> New Revision: 36913
>>
>> Log:
>> Fix race condition in ra_serf which can cause infinite-loop during a commit.
>> Diagnosis and patch by Jon Trowbridge <jon_at_trowbridge.org>.
>>
>> After initializing the parser, we were attempting to make it parse
>> response data.  Apparently up till now, the server response was always
>> "fast enough" such that 100% of the response was waiting in the
>> buffer, ready to be parsed, and so __handle_xml_parser() always
>> succeeded.
>>
>> But if you introduce latency into the server's response then parsing
>> might fail (the data may not be available yet), which means
>> 'parser.done' doesn't get set.  But then we *never* re-enter the
>> initialization block on subsequent calls, which means 'done' never
>> gets set, which means we're in an infinite loop waiting for 'done'.
>>
>> * subversion/libsvn_ra_serf/util.c
>>  (svn_ra_serf__handle_multistatus_only): separate parser
>>  initialization from the "is parser done?" check.  And actually examine
>>  the status result of __handle_xml_parser()!
>>
>> Modified:
>>   trunk/subversion/libsvn_ra_serf/util.c
>>
>> Modified: trunk/subversion/libsvn_ra_serf/util.c
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_ra_serf/util.c?pathrev=36913&r1=36912&r2=36913
>> ==============================================================================
>> --- trunk/subversion/libsvn_ra_serf/util.c      Tue Mar 31 13:39:01 2009        (r36912)
>> +++ trunk/subversion/libsvn_ra_serf/util.c      Tue Mar 31 14:22:56 2009        (r36913)
>> @@ -846,6 +846,7 @@ svn_ra_serf__handle_multistatus_only(ser
>>   svn_ra_serf__simple_request_context_t *ctx = baton;
>>   svn_ra_serf__server_error_t *server_err = &ctx->server_error;
>>
>> +  /* If necessary, initialize our XML parser. */
>>   if (server_err && !server_err->init)
>>     {
>>       serf_bucket_t *hdrs;
>> @@ -867,16 +868,7 @@ svn_ra_serf__handle_multistatus_only(ser
>>           server_err->parser.cdata = cdata_207;
>>           server_err->parser.done = &ctx->done;
>>           server_err->parser.ignore_errors = TRUE;
>> -
>> -          status = svn_ra_serf__handle_xml_parser(request, response,
>> -                                                  &server_err->parser, pool);
>> -
>> -          if (ctx->done && server_err->error->apr_err == APR_SUCCESS)
>> -            {
>> -              svn_error_clear(server_err->error);
>> -              server_err->error = SVN_NO_ERROR;
>> -            }
>> -        }
>> +       }
>>       else
>>         {
>>           ctx->done = TRUE;
>> @@ -884,6 +876,30 @@ svn_ra_serf__handle_multistatus_only(ser
>>         }
>>     }
>>
>> +  /* If server_err->error still contains APR_SUCCESS, it means that we
>> +     have not successfully parsed the XML yet. */
>> +  if (server_err && server_err->error
>> +      && server_err->error->apr_err == APR_SUCCESS)
>> +    {
>> +      status = svn_ra_serf__handle_xml_parser(request, response,
>> +                                             &server_err->parser, pool);
>> +
>> +      /* APR_EOF will be returned when parsing is complete.  If we see
>> +        any other error, return it immediately.  In practice the only
>> +        other error we expect to see is APR_EAGAIN, which indicates that
>> +        we could not parse the XML because the contents are not yet
>> +        available to be read. */
>> +      if (!APR_STATUS_IS_EOF(status))
>> +       {
>> +         return status;
>> +       }
>> +      else if (ctx->done && server_err->error->apr_err == APR_SUCCESS)
>> +       {
>> +         svn_error_clear(server_err->error);
>> +         server_err->error = SVN_NO_ERROR;
>> +       }
>> +    }
>> +
>>   status = svn_ra_serf__handle_discard_body(request, response,
>>                                             NULL, pool);
>>
>> ------------------------------------------------------
>> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=1499817
>>
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1499834
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1506592
Received on 2009-04-01 14:33:17 CEST