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

On 09/05/11 12:59, Philip Martin wrote:

> Yes, that would work. An alternative would be to deregister the pool
> cleanup on an explicit close.

Here is a version that does this. It's tested with the existing tests,
plus my small test case, and it stops the leaking.

[[[
Automatically svn_stream_close() any streams which are not 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       |    4 ++-
 subversion/libsvn_subr/stream.c                    |   26
++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 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..dd625a2 100644
--- a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
+++ b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
@@ -2123,7 +2123,9 @@ svn_swig_py_make_stream(PyObject *py_io,
apr_pool_t *pool)
   svn_stream_set_read(stream, read_handler_pyio);
   svn_stream_set_write(stream, write_handler_pyio);
   svn_stream_set_close(stream, close_handler_pyio);
-  Py_INCREF(py_io);
+  Py_INCREF(py_io); /* this will be decremented in the
+		       close_handler_pyio, which will be called when
+		       the pool containing this stream is cleared. */
   return stream;
 }
diff --git a/subversion/libsvn_subr/stream.c
b/subversion/libsvn_subr/stream.c
index f03f963..c7d6e41 100644
--- a/subversion/libsvn_subr/stream.c
+++ b/subversion/libsvn_subr/stream.c
@@ -54,11 +54,33 @@ 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 */
 };
 
 /*** Generic streams. ***/
+static apr_status_t
+close_stream_cleanup(void *stream)
+{
+  apr_status_t apr_err = APR_SUCCESS;
+  svn_error_t *err;
+
+  /* This is used to close the stream if the pool is being destroyed.
+     It was first introduced for the Python API, to allow implicit
+     closing.
+   */
+
+  err = svn_stream_close(stream);
+  if (err)
+    {
+      apr_err = err->apr_err;
+      svn_error_clear(err);
+    }
+
+  return apr_err;
+}
+
 svn_stream_t *
 svn_stream_create(void *baton, apr_pool_t *pool)
 {
@@ -73,6 +95,9 @@ svn_stream_create(void *baton, apr_pool_t *pool)
   stream->mark_fn = NULL;
   stream->seek_fn = NULL;
   stream->buffered_fn = NULL;
+  stream->pool = pool;
+  apr_pool_pre_cleanup_register(stream->pool, stream,
+				close_stream_cleanup);
   return stream;
 }
@@ -195,6 +220,7 @@ svn_stream_buffered(svn_stream_t *stream)
 svn_error_t *
 svn_stream_close(svn_stream_t *stream)
 {
+  apr_pool_cleanup_kill(stream->pool, stream, close_stream_cleanup);
   if (stream->close_fn == NULL)
     return SVN_NO_ERROR;
   return stream->close_fn(stream->baton);
]]]
-- 
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.