subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rhuij...@apache.org
Subject svn commit: r1557686 - in /subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h util.c
Date Mon, 13 Jan 2014 11:59:56 GMT
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;



Mime
View raw message