Nick Piper wrote on Mon, May 09, 2011 at 15:31:53 +0100:
> 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
s/go/go out/
> 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.
> + */
> +
The comment about history belongs in the log message, not in the code.
(the opposite of the usual problem :-))
> + 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);
Why did you switch to apr_pool_pre_cleanup_register() from
apr_pool_cleanup_register()?
> 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);
> ]]]
