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