You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2014/01/13 12:59:56 UTC
svn commit: r1557686 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h util.c
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/ra_serf.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h?rev=1557686&r1=1557685&r2=1557686&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Mon Jan 13 11:59:56 2014
@@ -440,6 +440,7 @@ typedef struct svn_ra_serf__handler_t {
/* Has the request/response been completed? */
svn_boolean_t done;
+ svn_boolean_t scheduled; /* Is the request scheduled in a context */
/* If we captured an error from the server, then this will be non-NULL.
It will be allocated from HANDLER_POOL. */
@@ -512,7 +513,6 @@ typedef struct svn_ra_serf__handler_t {
/* Pool for allocating SLINE.REASON and LOCATION. If this pool is NULL,
then the requestor does not care about SLINE and LOCATION. */
apr_pool_t *handler_pool;
-
} svn_ra_serf__handler_t;
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 */
}
return outer_status;
@@ -1538,24 +1525,29 @@ setup_request_cb(serf_request_t *request
void
svn_ra_serf__request_create(svn_ra_serf__handler_t *handler)
{
- SVN_ERR_ASSERT_NO_RETURN(handler->handler_pool != NULL);
-
- /* In case HANDLER is re-queued, reset the various transient fields.
+ SVN_ERR_ASSERT_NO_RETURN(handler->handler_pool != NULL
+ && !handler->scheduled);
- ### prior to recent changes, HANDLER was constant. maybe we should
- ### break out these processing fields, apart from the request
- ### definition. */
+ /* In case HANDLER is re-queued, reset the various transient fields. */
handler->done = FALSE;
handler->server_error = NULL;
handler->sline.version = 0;
handler->location = NULL;
handler->reading_body = FALSE;
handler->discard_body = FALSE;
+ handler->scheduled = TRUE;
+
+ /* Keeping track of the returned request object would be nice, but doesn't
+ work the way we would expect in ra_serf..
- /* ### do we ever alter the >response_handler? */
+ Serf sometimes creates a new request for us (and destroys the old one)
+ without telling, like when authentication failed (401/407 response.
- /* ### do we need to hold onto the returned request object, or just
- ### not worry about it (the serf ctx will manage it). */
+ We 'just' trust serf to do the right thing and expect it to tell us
+ when the state of the request changes.
+
+ ### I fixed a request leak in serf in r2258 on auth failures.
+ */
(void) serf_connection_request_create(handler->conn->conn,
setup_request_cb, handler);
}
@@ -1849,6 +1841,28 @@ response_done(serf_request_t *request,
return SVN_NO_ERROR;
}
+/* Pool cleanup handler for request handlers.
+
+ If a serf context run stops for some outside error, like when the user
+ cancels a request via ^C in the context loop, the handler is still
+ registered in the serf context. With the pool cleanup there would be
+ handlers registered in no freed memory.
+
+ This fallback kills the connection for this case, which will make serf
+ unregister any*/
+static apr_status_t
+handler_cleanup(void *baton)
+{
+ svn_ra_serf__handler_t *handler = baton;
+ if (handler->scheduled && handler->conn)
+ {
+ SVN_DBG(("Resetting connection!"));
+ serf_connection_reset(handler->conn->conn);
+ }
+
+ return APR_SUCCESS;
+}
+
svn_ra_serf__handler_t *
svn_ra_serf__create_handler(apr_pool_t *result_pool)
{
@@ -1857,6 +1871,9 @@ svn_ra_serf__create_handler(apr_pool_t *
handler = apr_pcalloc(result_pool, sizeof(*handler));
handler->handler_pool = result_pool;
+ apr_pool_cleanup_register(result_pool, handler, handler_cleanup,
+ apr_pool_cleanup_null);
+
/* Setup the default done handler, to handle server errors */
handler->done_delegate_baton = handler;
handler->done_delegate = response_done;
Re: svn commit: r1557686 - in /subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h util.c
Posted by Bert Huijben <be...@qqmail.nl>.
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@subversion.apache.org
Bert,
On Mon, Jan 13, 2014 at 12:59 PM, <rh...@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
Re: svn commit: r1557686 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h util.c
Posted by Lieven Govaerts <sv...@mobsol.be>.
Bert,
On Mon, Jan 13, 2014 at 12:59 PM, <rh...@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