Nick Piper wrote on Mon, May 09, 2011 at 11:21:10 +0100:
>
>
> On 09/05/11 09:05, Markus Schaber wrote:
> > Hi, Hick,
> >
> >> Von: Piper, Nick [mailto:nick.pi...@logica.com]
> >
> >> In Python, f is a "file" instance, but that will be converted to a
> >> svn_stream_t by swig (by svn_swig_py_make_stream()).
> >>
> >> That file then goes out of scope in Python, but the file descriptor is
> >> never closed. I don't see where we should be calling
> > svn_stream_close() in
> >> order to do that (and call Py_DECREF(py_io)), because the svn_stream_t
> > is
> >> never exposed back to Python.
> >
> > I'm not sure about what svn_swig_py_make_stream() does, but normal
> > python files automatically close the underlying os resources (file
> > descriptor etc.) when they are garbage collected.
>
> I feel in this case, the reference counting means that the gc won't
> happen - Py_INCREF(py_io) at
> https://github.com/apache/subversion/blob/trunk/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c#L2126
> increases it by one, but the corresponding Py_DECREF(py_io) at
> https://github.com/apache/subversion/blob/trunk/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c#L2112
> is never called.
>
> I'll continue to study this to see if I'm correct or not....
>
> Nick
>
Restating Nick's issue: the PyObject wrapped by the svn_stream_t created
by svn_swig_py_make_stream() goes out of scope in Python, but the stream
maintains a reference to it so the PyObject doesn't get destroyed/deleted.
As he says, calling the stream's close handler would Py_DECREF the Python
object and thus, hopefully, allow it to be removed.
I've suggested Nick to install a pool cleanup handler on the stream to
ensure that it's closed:
[[[
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1100957)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -59,6 +59,22 @@ struct svn_stream_t {
/*** Generic streams. ***/
+static apr_status_t
+close_stream_cleanup(void *stream)
+{
+ apr_status_t apr_err = APR_SUCCESS;
+ svn_error_t *err;
+
+ 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 +89,9 @@ svn_stream_create(void *baton, apr_pool_t *pool)
stream->mark_fn = NULL;
stream->seek_fn = NULL;
stream->buffered_fn = NULL;
+ apr_pool_cleanup_register(pool, stream,
+ close_stream_cleanup,
+ apr_pool_cleanup_null);
return stream;
}
]]]
