[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: AW: Python bindings: Use of svn_swig_py_make_stream() - avoiding leaking fds

From: Nick Piper <nick.piper_at_logica.com>
Date: Tue, 10 May 2011 12:55:35 +0100

On 09/05/11 21:51, Julian Foad wrote:
> Nick Piper wrote:
>> Add svn_stream_close_on_cleanup(), which will register a stream to be
>> automatically closed when the pool is destroyed.
>
> First impression: the public name makes it sound like a user-level
> thing, but this is for use by a stream's implementation, right? Not by
> its users.
>
> I don't think that works as a part of the generic stream model. The
> "close" method on any stream is required to call the "close" method of
> its underlying stream, but that's not what we want in the case of an
> unexpected close (which we could call an "abort"). If we have a
> checksumming stream (which reads to the end on close and so doesn't want
> its close method to be called on abort), and this stream is wrapped by
> some kind of filtering stream (which allocates some resources and
> chooses to have its "close" method called on abort), then if we abort we
> would call filtering.close() which necessarily calls
> checksumming.close() even though we didn't want that.

I see, that makes sense.

> Instead I think we need the ability for a stream to provide an "abort"
> method, which will be called on pool clean-up and which is not required
> to call the underlying stream's close() but is merely required to free
> up any resources it may have allocated itself.

I've named it "cleanup" rather than "abort", because abort makes me feel
like it's some error situation. In the case of the Python bindings, it's
not any error, it's just normal cleanup that needs to happen.

As my mailer munges long lines and I've not fixed that yet, I've
attached a revised patch. I hope it gets the mimetype correct, this is
not my normal mail client.

I need help with it, I'm not certain the best way for the error handling
to work. There is a comment in the patch:

+/*
+ The cleanup function we register with apr_pool wants to return
+ apr_status_t, but to match the other callback-functions, the
+ user-provided cleanup_fn wants to return svn_error_t.
+
+ So I thought of going through this extra step, having apr call
+ stream_cleanup() rather than cleanup_fn() directly; but I'm not sure
+ how to convert the error.
+ */

Maybe it's simplier just to make svn_cleanup_fn_t return an
apr_status_t, even though this doesn't match the other callback function
signatures, such as svn_close_fn_t. Also see possibly-wrong error
handling in svn_stream_close().

 Nick

-- 
Sorry for this disclaimer:
Think green - keep it on the screen.
This e-mail and any attachment is for authorised use by the intended recipient(s) only. It may contain proprietary material, confidential information and/or be subject to legal privilege. It should not be copied, disclosed to, retained or used by, any other party. If you are not an intended recipient then please promptly delete this e-mail and any attachment and all copies and inform the sender. Thank you. 
Received on 2011-05-10 13:56:57 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.