> -----Original Message-----
> From: Daniel Shahaf [mailto:danie...@elego.de]
> Sent: maandag 9 mei 2011 17:31
> To: Nick Piper
> Cc: dev@subversion.apache.org
> Subject: Re: AW: Python bindings: Use of svn_swig_py_make_stream() -
> avoiding leaking fds
>
> Nick Piper wrote on Mon, May 09, 2011 at 16:19:23 +0100:
> > Thanks for reviewing.
> >
> > On 09/05/11 15:53, Daniel Shahaf wrote:
> > > Nick Piper wrote on Mon, May 09, 2011 at 15:31:53 +0100:
> >
> > >> + /* 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 :-))
> >
> > Should we keep the first line of it?
> >
>
> It would make a good docstring.
>
> > >> + 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()?
> >
> > Otherwise tests segfault, with backtrace:
> >
> > [...]
> > #0 0xb6ff7901 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
> > #1 0xb6ff7934 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
> > #2 0xb6ff7934 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
> > [...]
> > #174381 0xb6ff7934 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
> > #174382 0xb6ff7934 in apr_pool_destroy () from /usr/lib/libapr-1.so.0
> > #174383 0xb6ff7844 in apr_pool_clear () from /usr/lib/libapr-1.so.0
> > #174384 0xb6daca86 in write_final_rev (new_id_p=0xbf8eed20,
> > file=0x90826e8, rev=4, fs=0x905bfa8, id=0x90b4498, start_node_id=0x0,
> > start_copy_id=0x0, initial_offset=231, reps_to_cache=0x909e358,
> > reps_pool=0x9066f30, pool=0x9101810)
> > at subversion/libsvn_fs_fs/fs_fs.c:6050
> > #174385 0xb6dacaeb in write_final_rev (new_id_p=0xbf8eef40,
> > file=0x90826e8, rev=4, fs=0x905bfa8, id=0x90842f8, start_node_id=0x0,
> > start_copy_id=0x0, initial_offset=231, reps_to_cache=0x909e358,
> > reps_pool=0x9066f30, pool=0x90b2cd0)
> > at subversion/libsvn_fs_fs/fs_fs.c:6051
> > [...]
> >
> > I'm not familiar with the pool API, but reading
> >
> http://apr.apache.org/docs/apr/1.3/group___pool_cleanup.html#g64114542
> 989d8872c7fd3c62f2a68678
> > suggests that if we want to use the data that's in the pool during the
> > cleanup, we have to do that in pre.
> >
>
> Fair enough. I'll run tests too, etc, and commit.
Why fix this in the generic stream?
This changes behavior all over the place. Especially on wrapping streams
like our checksum streams which may now start reading GBytes of data while
we are just closing.
Our usual pattern is to only add cleanups where we need it (E.g. on the
enclosed apr file)
Bert
