Bert Huijben wrote on Mon, May 09, 2011 at 17:49:52 +0200:
>
>
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:danielsh_at_elego.de]
> > Sent: maandag 9 mei 2011 17:31
> > To: Nick Piper
> > Cc: dev_at_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)
>
On IRC Bert convinced me that this shouldn't be teh default behaviour
for all streams via the example of reading two streams to compare their
contents for equality.
> Bert
>
>
>
Received on 2011-05-09 18:04:04 CEST