RFT: issue 1164 patch

Howdy,

The following patch addresses issue 1164. I have tested it with
ra_local, and am currently testing with ra_svn. Would someone
please test it with ra_dav?

I'll remove the tabs before checking it in. Other feedback
welcome.

Matt

-- 
Oink!
Index: subversion/include/svn_ra.h
===================================================================
--- subversion/include/svn_ra.h	(revision 5192)
+++ subversion/include/svn_ra.h	(working copy)
@@ -274,14 +274,6 @@
                         apr_hash_t *config,
                         apr_pool_t *pool);
 
-  /** Close a repository session. 
-   *
-   * Close a repository session.  This frees any memory used by the
-   * session baton.  (To free the session baton itself, simply free
-   * the pool it was created in.)
-   */
-  svn_error_t *(*close) (void *session_baton);
-
   /** Get the latest revision number from the repository.
    *
    * Get the latest revision number from the repository. This is
Index: subversion/libsvn_ra_local/ra_plugin.c
===================================================================
--- subversion/libsvn_ra_local/ra_plugin.c	(revision 5192)
+++ subversion/libsvn_ra_local/ra_plugin.c	(working copy)
@@ -264,23 +264,6 @@
 
 
 static svn_error_t *
-svn_ra_local__close (void *session_baton)
-{
-  svn_ra_local__session_baton_t *baton = session_baton;
-
-  /* ### maybe arrange to have a pool which can be cleared... */
-
-  /* People shouldn't try and use these objects now. */
-  baton->repos = NULL;
-  baton->fs = NULL;
-
-  return SVN_NO_ERROR;
-}
-
-
-
-
-static svn_error_t *
 svn_ra_local__get_latest_revnum (void *session_baton,
                                  svn_revnum_t *latest_revnum)
 {
@@ -947,7 +930,6 @@
   "ra_local",
   "Module for accessing a repository on local disk.",
   svn_ra_local__open,
-  svn_ra_local__close,
   svn_ra_local__get_latest_revnum,
   svn_ra_local__get_dated_revision,
   svn_ra_local__change_rev_prop,
Index: subversion/libsvn_client/prop_commands.c
===================================================================
--- subversion/libsvn_client/prop_commands.c	(revision 5192)
+++ subversion/libsvn_client/prop_commands.c	(working copy)
@@ -167,9 +167,6 @@
   /* The actual RA call. */
   SVN_ERR (ra_lib->change_rev_prop (session, *set_rev, propname, propval));
 
-  /* All done. */
-  SVN_ERR (ra_lib->close(session));
-
   return SVN_NO_ERROR;
 }
 
@@ -522,9 +519,6 @@
           return svn_error_create
             (SVN_ERR_CLIENT_BAD_REVISION, NULL, "unknown revision kind");
         }
-
-      /* Close the RA session. */
-      SVN_ERR (ra_lib->close (session));
     }
   else  /* working copy path */
     {
@@ -610,9 +604,6 @@
   /* The actual RA call. */
   SVN_ERR (ra_lib->rev_prop (session, *set_rev, propname, propval));
 
-  /* All done. */
-  SVN_ERR (ra_lib->close(session));
-
   return SVN_NO_ERROR;
 }
 
@@ -910,9 +901,6 @@
           return svn_error_create
             (SVN_ERR_CLIENT_BAD_REVISION, NULL, "unknown revision kind");
         }
-
-      /* Close the RA session. */
-      SVN_ERR (ra_lib->close (session));
     }
   else  /* working copy path */
     {
@@ -992,9 +980,6 @@
       apr_hash_this (hi, &key, &klen, &val);
       apr_hash_set (proplist, key, klen, val);
     } 
-
-  /* All done. */
-  SVN_ERR (ra_lib->close(session));
   
   *props = proplist;
   return SVN_NO_ERROR;
Index: subversion/libsvn_client/switch.c
===================================================================
--- subversion/libsvn_client/switch.c	(revision 5192)
+++ subversion/libsvn_client/switch.c	(working copy)
@@ -293,9 +293,6 @@
   if (err)
     return err;
 
-  /* Close the RA session. */
-  SVN_ERR (ra_lib->close (session));
-
   SVN_ERR (svn_wc_adm_close (adm_access));
 
   return SVN_NO_ERROR;
Index: subversion/libsvn_client/delete.c
===================================================================
--- subversion/libsvn_client/delete.c	(revision 5192)
+++ subversion/libsvn_client/delete.c	(working copy)
@@ -189,9 +189,6 @@
                                                    committed_author,
                                                    committed_date,
                                                    pool);
-      
-      /* Free the RA session */
-      SVN_ERR (ra_lib->close (session));
 
       return SVN_NO_ERROR;
     }
Index: subversion/libsvn_client/checkout.c
===================================================================
--- subversion/libsvn_client/checkout.c	(revision 5192)
+++ subversion/libsvn_client/checkout.c	(working copy)
@@ -123,9 +123,6 @@
           return err;
         }
       *use_sleep = TRUE;
-
-      /* Close the RA session. */
-      SVN_ERR (ra_lib->close (session));
     }      
   
   /* We handle externals after the initial checkout is complete, so
Index: subversion/libsvn_client/cat.c
===================================================================
--- subversion/libsvn_client/cat.c	(revision 5192)
+++ subversion/libsvn_client/cat.c	(working copy)
@@ -132,7 +132,5 @@
       SVN_ERR (svn_stream_close (tmp_stream));
     }
 
-  SVN_ERR (ra_lib->close (session));
-
   return SVN_NO_ERROR;
 }
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c	(revision 5192)
+++ subversion/libsvn_client/diff.c	(working copy)
@@ -859,10 +859,6 @@
   
   SVN_ERR (reporter->finish_report (report_baton));
   
-  SVN_ERR (ra_lib->close (session2));
-
-  SVN_ERR (ra_lib->close (session));
-
   return SVN_NO_ERROR;
 }
 
@@ -915,7 +911,6 @@
   SVN_ERR (svn_client__get_revision_number
            (&rev1, ra_lib, session1, revision1, NULL, pool));
   SVN_ERR (ra_lib->get_file (session1, "", rev1, fstream1, NULL, &props1));
-  SVN_ERR (ra_lib->close (session1));
 
   /* ### heh, funny.  we could be fetching two fulltexts from two
      *totally* different repositories here.  :-) */
@@ -927,7 +922,6 @@
   SVN_ERR (svn_client__get_revision_number
            (&rev2, ra_lib, session2, merge_b->revision, NULL, pool));
   SVN_ERR (ra_lib->get_file (session2, "", rev2, fstream2, NULL, &props2));
-  SVN_ERR (ra_lib->close (session2));
 
   status = apr_file_close (fp1);
   if (status)
@@ -1249,9 +1243,6 @@
           target1 = NULL;
         }
 
-      SVN_ERR (ra_lib->close (session));
-      SVN_ERR (ra_lib->close (session2));
-      
       /* The main session is opened to the anchor of URL1. */
       SVN_ERR (svn_client__open_ra_session (&session, ra_lib, anchor1,
                                             auth_dir,
@@ -1297,9 +1288,6 @@
 
       SVN_ERR (reporter->set_path (report_baton, "", start_revnum, pool));
       SVN_ERR (reporter->finish_report (report_baton));
-
-      SVN_ERR (ra_lib->close (session2));
-      SVN_ERR (ra_lib->close (session));      
     }
 
   else
Index: subversion/libsvn_client/copy.c
===================================================================
--- subversion/libsvn_client/copy.c	(revision 5192)
+++ subversion/libsvn_client/copy.c	(working copy)
@@ -435,8 +435,6 @@
                                                committed_date,
                                                pool);
 
-  SVN_ERR (ra_lib->close (sess));
-
   return SVN_NO_ERROR;
 }
 
@@ -578,11 +576,6 @@
                                svn_path_uri_decode (target, pool),
                                SVN_INVALID_REVNUM));
   
-  /* Close the RA session.  We'll re-open it after we've figured out
-     the right URL to open. */
-  SVN_ERR (ra_lib->close (session));
-  session = NULL;
-
   /* BASE_URL defaults to DST_URL. */
   base_url = apr_pstrdup (pool, dst_url);
   if (dst_kind == svn_node_none)
@@ -667,10 +660,6 @@
   if (commit_in_progress)
     editor->abort_edit (edit_baton, pool); /* ignore return value */
 
-  /* We were committing to RA, so close the session. */
-  if (session)
-    ra_lib->close (session);
-
   /* ### Under what conditions should we remove the locks? */
   unlock_err = svn_wc_adm_close (adm_access);
 
@@ -875,9 +864,6 @@
         src_revnum = fetched_rev;
     }
 
-  /* Free the RA session. */
-  SVN_ERR (ra_lib->close (sess));
-      
   /* Schedule the new item for addition-with-history.
 
      If the new item is a directory, the URLs will be recursively
Index: subversion/libsvn_client/ls.c
===================================================================
--- subversion/libsvn_client/ls.c	(revision 5192)
+++ subversion/libsvn_client/ls.c	(working copy)
@@ -115,8 +115,6 @@
 
       SVN_ERR (get_dir_contents (*dirents, "", rev, ra_lib, session, recurse,
                                  pool));
-
-      SVN_ERR (ra_lib->close (session));
     }
   else if (url_kind == svn_node_file)
     {
@@ -126,7 +124,6 @@
 
       /* Re-open the session to the file's parent instead. */
       svn_path_split (url, &parent_url, &base_name, pool);
-      SVN_ERR (ra_lib->close (session));
       SVN_ERR (svn_client__open_ra_session (&session, ra_lib, parent_url,
                                             auth_dir,
                                             NULL, NULL, FALSE, TRUE, 
@@ -139,8 +136,6 @@
         return svn_error_create (SVN_ERR_RA_NOT_IMPLEMENTED, NULL,
                                  "No get_dir() available for url schema.");
 
-      SVN_ERR (ra_lib->close (session));
-
       /* Copy the relevant entry into the caller's hash. */
       *dirents = apr_hash_make (pool);
       the_ent = apr_hash_get (parent_ents, base_name, APR_HASH_KEY_STRING);
Index: subversion/libsvn_client/log.c
===================================================================
--- subversion/libsvn_client/log.c	(revision 5192)
+++ subversion/libsvn_client/log.c	(working copy)
@@ -289,8 +289,5 @@
       }
   }
   
-  /* We're done with the RA session. */
-  SVN_ERR (ra_lib->close (session));
-
   return err;
 }
Index: subversion/libsvn_client/update.c
===================================================================
--- subversion/libsvn_client/update.c	(revision 5192)
+++ subversion/libsvn_client/update.c	(working copy)
@@ -149,9 +149,6 @@
           return err;
         }
       *use_sleep = TRUE;
-
-      /* Close the RA session. */
-      SVN_ERR (ra_lib->close (session));
     }      
   
   /* We handle externals after the update is complete, so that
Index: subversion/libsvn_client/status.c
===================================================================
--- subversion/libsvn_client/status.c	(revision 5192)
+++ subversion/libsvn_client/status.c	(working copy)
@@ -129,9 +129,6 @@
                                    NULL,
                                    pool));
 
-  /* We're done with the RA session. */
-  SVN_ERR (ra_lib->close (session));
-
   return SVN_NO_ERROR;
 }
 
Index: subversion/libsvn_client/add.c
===================================================================
--- subversion/libsvn_client/add.c	(revision 5192)
+++ subversion/libsvn_client/add.c	(working copy)
@@ -241,9 +241,6 @@
                                                    committed_date,
                                                    pool);
 
-      /* Free the RA session. */
-      SVN_ERR (ra_lib->close (session));
-
       return SVN_NO_ERROR;
     }
 
Index: subversion/libsvn_client/commit.c
===================================================================
--- subversion/libsvn_client/commit.c	(revision 5192)
+++ subversion/libsvn_client/commit.c	(working copy)
@@ -610,9 +610,6 @@
       return err;
     }
 
-  /* Close the session. */
-  SVN_ERR (ra_lib->close (session));
-
   /* Finally, fill in the commit_info structure. */
   *commit_info = svn_client__make_commit_info (committed_rev,
                                                committed_author,
@@ -964,10 +961,6 @@
         svn_pool_destroy (subpool);
     }
 
-  /* Close the RA session. */
-  if ((cleanup_err = ra_lib->close (session)))
-    goto cleanup;
-
   /* Sleep to ensure timestamp integrity. */
   svn_sleep_for_timestamps ();
 
Index: subversion/libsvn_ra_svn/client.c
===================================================================
--- subversion/libsvn_ra_svn/client.c	(revision 5192)
+++ subversion/libsvn_ra_svn/client.c	(working copy)
@@ -95,6 +95,12 @@
   return 0;
 }
 
+/* A pool cleanup handler to close a socket. */
+static apr_status_t cleanup_socket(void *arg)
+{
+  return apr_socket_close(arg);
+}
+
 static svn_error_t *make_connection(const char *hostname, unsigned short port,
                                     apr_socket_t **sock, apr_pool_t *pool)
 {
@@ -116,6 +122,9 @@
     return svn_error_createf(status, NULL, "Can't connect to host '%s'",
                              hostname);
 
+  apr_pool_cleanup_register(pool, *sock, cleanup_socket,
+			    apr_pool_cleanup_null);
+
   return SVN_NO_ERROR;
 }
 
@@ -282,6 +291,14 @@
   return apr_file_close(arg);
 }
 
+/* A pool cleanup handler to wait for a process. */
+static apr_status_t cleanup_process(void *arg)
+{
+  while (apr_proc_wait(arg, NULL, NULL, APR_WAIT) == APR_CHILD_NOTDONE)
+    ;
+  return APR_SUCCESS;
+}
+
 static svn_error_t *ra_svn_open(void **sess, const char *url,
                                 const svn_ra_callbacks_t *callbacks,
                                 void *callback_baton,
@@ -325,10 +342,9 @@
        * to avoid that); this means we have to be careful not to leave
        * stuff lying around in the write buffer between ra_lib
        * calls. */
-      apr_pool_cleanup_register(pool, proc->in, apr_pool_cleanup_null,
-                                cleanup_file);
-      apr_pool_cleanup_register(pool, proc->out, apr_pool_cleanup_null,
-                                cleanup_file);
+      apr_pool_cleanup_register(pool, proc->in, cleanup_file, cleanup_file);
+      apr_pool_cleanup_register(pool, proc->out, cleanup_file, cleanup_file);
+      apr_pool_cleanup_register(pool, proc, cleanup_process, apr_pool_cleanup_null);
     }
   else
     {
@@ -382,24 +398,6 @@
   return SVN_NO_ERROR;
 }
 
-static svn_error_t *ra_svn_close(void *sess)
-{
-  svn_ra_svn_conn_t *conn = sess;
-
-  if (conn->sock)
-    apr_socket_close(conn->sock);
-  else
-    {
-      apr_file_close(conn->in_file);
-      apr_file_close(conn->out_file);
-      /* ### Perhaps a cleanup handler should make sure this gets done? */
-      while (apr_proc_wait(conn->proc, NULL, NULL,
-                           APR_WAIT) == APR_CHILD_NOTDONE)
-        ;
-    }
-  return SVN_NO_ERROR;
-}
-
 static svn_error_t *ra_svn_get_latest_rev(void *sess, svn_revnum_t *rev)
 {
   svn_ra_svn_conn_t *conn = sess;
@@ -854,7 +852,6 @@
   "ra_svn",
   "Module for accessing a repository using the svn network protocol.",
   ra_svn_open,
-  ra_svn_close,
   ra_svn_get_latest_rev,
   ra_svn_get_dated_rev,
   ra_svn_change_rev_prop,
Index: subversion/libsvn_ra_dav/session.c
===================================================================
--- subversion/libsvn_ra_dav/session.c	(revision 5192)
+++ subversion/libsvn_ra_dav/session.c	(working copy)
@@ -45,6 +45,13 @@
   return APR_SUCCESS;
 }
 
+/* a cleanup routine attached to the pool that contains the RA session
+   root URI. */
+static apr_status_t cleanup_uri(void *uri)
+{
+  ne_uri_free(uri);
+  return APR_SUCCESS;
+}
 
 /* A neon-session callback to 'pull' authentication data when
    challenged.  In turn, this routine 'pulls' the data from the client
@@ -465,13 +472,16 @@
   ras = apr_pcalloc(pool, sizeof(*ras));
   ras->pool = pool;
   ras->url = apr_pstrdup (pool, repos_URL);
-  ras->root = uri; /* copies uri pointer members, they get free'd in __close. */
+  ras->root = uri;
   ras->sess = sess;
   ras->sess2 = sess2;  
   ras->callbacks = callbacks;
   ras->callback_baton = callback_baton;
   ras->compression = compression;
 
+  /* make sure we eventually destroy the uri */
+  apr_pool_cleanup_register(pool, &ras->root, cleanup_uri, apr_pool_cleanup_null);
+
   /* note that ras->username and ras->password are still NULL at this
      point. */
 
@@ -488,17 +498,6 @@
 
 
 
-static svn_error_t *svn_ra_dav__close (void *session_baton)
-{
-  svn_ra_session_t *ras = session_baton;
-
-  (void) apr_pool_cleanup_run(ras->pool, ras->sess, cleanup_session);
-  (void) apr_pool_cleanup_run(ras->pool, ras->sess2, cleanup_session);
-  ne_uri_free(&ras->root);
-  return NULL;
-}
-
-
 static svn_error_t *svn_ra_dav__do_get_uuid(void *session_baton,
                                             const char **uuid)
 {
@@ -523,7 +522,6 @@
   "ra_dav",
   "Module for accessing a repository via WebDAV (DeltaV) protocol.",
   svn_ra_dav__open,
-  svn_ra_dav__close,
   svn_ra_dav__get_latest_revnum,
   svn_ra_dav__get_dated_revision,
   svn_ra_dav__change_rev_prop,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org