subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lieven Govaerts <svn...@mobsol.be>
Subject Re: svn commit: r1557686 - in /subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h util.c
Date Sun, 22 Jun 2014 08:32:44 GMT
Bert,

On Mon, Jan 13, 2014 at 12:59 PM,  <rhuijben@apache.org> wrote:
> Author: rhuijben
> Date: Mon Jan 13 11:59:56 2014
> New Revision: 1557686
>
> URL: http://svn.apache.org/r1557686
> Log:
> Make ra_serf use a pool cleanup handler on its request handlers to allow
> reusing the ra session in cases that before this patch would cause a segfault
> because it wasn't cleaned up.
>
> If the ra session would schedule a new request before timeout, the context
> would continue delivering data to old handlers. After this patch that won't
> happen again.
>
> * subversion/libsvn_ra_serf/ra_serf.h
>   (svn_ra_serf__handler_t): Add boolean.
>
> * subversion/libsvn_ra_serf/util.c
>   (svn_ra_serf__context_run_one): Remove specialized handling of
>     SVN_ERR_CEASE_INVOCATION that was already not required after recent
>     error handling cleanups, but is now completely unneeded.
>   (handle_response): Unregister on request errors.
>   (handle_response_cb): Unregister on EOF and/or errors.
>   (svn_ra_serf__request_create): Check for double scheduling. Update comment
>     to match current reality.
>
>   (handler_cleanup): New function.
>   (svn_ra_serf__create_handler): Register handler_cleanup.
>
> Modified:
>     subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
>     subversion/trunk/subversion/libsvn_ra_serf/util.c
>
[..]

> Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1557686&r1=1557685&r2=1557686&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Mon Jan 13 11:59:56 2014
> @@ -930,23 +930,6 @@ svn_ra_serf__context_run_one(svn_ra_serf
>    /* Wait until the response logic marks its DONE status.  */
>    err = svn_ra_serf__context_run_wait(&handler->done, handler->session,
>                                        scratch_pool);
> -
> -  /* A callback invocation has been canceled. In this simple case of
> -     context_run_one, we can keep the ra-session operational by resetting
> -     the connection.
> -
> -     If we don't do this, the next context run will notice that the connection
> -     is still in the error state and will just return SVN_ERR_CEASE_INVOCATION
> -     (=the last error for the connection) again  */
> -  if (err && err->apr_err == SVN_ERR_CEASE_INVOCATION)
> -    {
> -      apr_status_t status = serf_connection_reset(handler->conn->conn);
> -
> -      if (status)
> -        err = svn_error_compose_create(err,
> -                                       svn_ra_serf__wrap_err(status, NULL));
> -    }
> -
>    return svn_error_trace(err);
>  }
>
> @@ -1200,6 +1183,8 @@ handle_response(serf_request_t *request,
>    if (!response)
>      {
>        /* Uh-oh. Our connection died.  */
> +      handler->scheduled = FALSE;
> +
>        if (handler->response_error)
>          {
>            /* Give a handler chance to prevent request requeue. */
> @@ -1423,6 +1408,7 @@ handle_response_cb(serf_request_t *reque
>      {
>        svn_ra_serf__session_t *sess = handler->session;
>        handler->done = TRUE;
> +      handler->scheduled = FALSE;
>        outer_status = APR_EOF;
>
>        /* We use a cached handler->session here to allow handler to free the
> @@ -1436,7 +1422,8 @@ handle_response_cb(serf_request_t *reque
>      {
>        handler->discard_body = TRUE; /* Discard further data */
>        handler->done = TRUE; /* Mark as done */
> -      return APR_EAGAIN; /* Exit context loop */
> +      handler->scheduled = FALSE;
> +      outer_status = APR_EAGAIN; /* Exit context loop */

This looks wrong: why are you hiding the error status from serf?

Is there an issue in serf that you want to work around here?

[..]

Lieven

Mime
View raw message