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

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
Received on 2009-03-31 23:26:03 CEST