Bert Huijben wrote on Mon, May 09, 2011 at 17:49:52 +0200: > > > > -----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) >
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 > > >
