Re: AW: Python bindings: Use of svn_swig_py_make_stream() - avoiding leaking fds

This version:
 * Makes the new behaviour optional, via stream_close_on_cleanup()
(which the Python API then uses.) (Thanks Bert.)
 * Uses apr_pool_cleanup_register() rather than the 'pre' variant, in
order to match the reset of svn and be compatible with older APR (thanks
Philip.)

[[[
Add svn_stream_close_on_cleanup(), which will register a stream to be
automatically closed when the pool is destroyed.

This is particularly needed for the Python bindings. The PyObject
wrapped by the svn_stream_t created by svn_swig_py_make_stream() might
go scope in Python, but the stream maintains a reference to it so the
PyObject doesn't get destroyed/deleted.

---
 .../swig/python/libsvn_swig_py/swigutil_py.c       |    1 +
 subversion/include/svn_io.h                        |    7 +++
 subversion/libsvn_subr/stream.c                    |   49
++++++++++++++++----
 3 files changed, 48 insertions(+), 9 deletions(-)
diff --git
a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
index d8b9c08..fff5e90 100644
--- a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
+++ b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
@@ -2124,6 +2124,7 @@ svn_swig_py_make_stream(PyObject *py_io,
apr_pool_t *pool)
   svn_stream_set_write(stream, write_handler_pyio);
   svn_stream_set_close(stream, close_handler_pyio);
   Py_INCREF(py_io);
+  svn_stream_close_on_cleanup(stream);
   return stream;
 }
diff --git a/subversion/include/svn_io.h b/subversion/include/svn_io.h
index c518d24..686f462 100644
--- a/subversion/include/svn_io.h
+++ b/subversion/include/svn_io.h
@@ -1092,6 +1092,13 @@ svn_stream_write(svn_stream_t *stream,
 svn_error_t *
 svn_stream_close(svn_stream_t *stream);
+/** Set @a stream to be automatically closed when pool containing it is
cleared
+ *
+ * @since New in 1.7
+ */
+void
+svn_stream_close_on_cleanup(svn_stream_t *stream);
+
 /** Reset a generic stream back to its origin. E.g. On a file this would be
  * implemented as a seek to position 0).  This function returns a
  * #SVN_ERR_STREAM_RESET_NOT_SUPPORTED error when the stream doesn't
diff --git a/subversion/libsvn_subr/stream.c
b/subversion/libsvn_subr/stream.c
index f03f963..0d4cfa5 100644
--- a/subversion/libsvn_subr/stream.c
+++ b/subversion/libsvn_subr/stream.c
@@ -54,6 +54,7 @@ struct svn_stream_t {
   svn_io_mark_fn_t mark_fn;
   svn_io_seek_fn_t seek_fn;
   svn_io_buffered_fn_t buffered_fn;
+  apr_pool_t *pool; /* to allow us to deregister clean up functions */
 };
 
@@ -73,9 +74,48 @@ svn_stream_create(void *baton, apr_pool_t *pool)
   stream->mark_fn = NULL;
   stream->seek_fn = NULL;
   stream->buffered_fn = NULL;
+  stream->pool = pool;
   return stream;
 }
+static svn_error_t *
+svn_stream_close_body(svn_stream_t *stream)
+{
+  if (stream->close_fn == NULL)
+    return SVN_NO_ERROR;
+  return stream->close_fn(stream->baton);
+}
+
+static apr_status_t
+close_stream_cleanup(void *stream)
+{
+  apr_status_t apr_err = APR_SUCCESS;
+  svn_error_t *err;
+
+  err = svn_stream_close_body(stream);
+  if (err)
+    {
+      apr_err = err->apr_err;
+      svn_error_clear(err);
+    }
+
+  return apr_err;
+}
+
+svn_error_t *
+svn_stream_close(svn_stream_t *stream)
+{
+  apr_pool_cleanup_kill(stream->pool, stream, close_stream_cleanup);
+  return svn_stream_close_body(stream);
+}
+
+void
+svn_stream_close_on_cleanup(svn_stream_t *stream)
+{
+  apr_pool_cleanup_register(stream->pool, stream,
+			    close_stream_cleanup,
+			    apr_pool_cleanup_null);
+}
 void
 svn_stream_set_baton(svn_stream_t *stream, void *baton)
@@ -193,15 +233,6 @@ svn_stream_buffered(svn_stream_t *stream)
 }
 svn_error_t *
-svn_stream_close(svn_stream_t *stream)
-{
-  if (stream->close_fn == NULL)
-    return SVN_NO_ERROR;
-  return stream->close_fn(stream->baton);
-}
-
-
-svn_error_t *
 svn_stream_printf(svn_stream_t *stream,
                   apr_pool_t *pool,
                   const char *fmt,
]]]
-- 
Sorry for this disclaimer:
Think green - keep it on the screen.
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you.