Once we return ‘an error code’, serf will just continue returning this error. So after’ the error’ the Ra session is completely unusable for further requests.
The behavior towards serfs is correct in that there was really no error in parsing the http response… But we determined that the client want to break out the context loop.
If we then support some SVN_ERR_SOMETHING then serf assumes the http connection is no longer usable, while we (as subversion) can just issue new requests on the same connection after some err 500 that makes the Ra function return an error.
The EAGAIN properly exits from the context, and then the Ra-serf loop returns the error. If another Ra-session operation is used we don't have to reset the whole connection, but can just use it. Code like merge would otherwise trash connections in a lot of cases.
And then we would even trigger more cases where serf (at the time) triggered lost requests.
Bert
Sent from Windows Mail
From: Lieven Govaerts
Sent: Sunday, June 22, 2014 10:32 AM
To: dev_at_subversion.apache.org
Bert,
On Mon, Jan 13, 2014 at 12:59 PM, <rhuijben_at_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
Received on 2014-06-22 22:26:53 CEST