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

> -----Original Message-----
> From: Julian Foad [mailto:julian.foad_at_wandisco.com]
> Sent: maandag 9 mei 2011 22:51
> 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:
> > 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.

Well in some cases it is a user level thing.

Swig python wants to receive a close event to release resources, but our streams don't close in this specific case.
(Because in all the other cases we do close it or we don't need a close).

Earlier I suggesting such a function, to allow the python bindings to request this close on pool cleanup.

(And we had this discussion earlier... when we were discussing if we should use the close event to install a new pristine file)

We kept the definition of streams very generic in the past (read: before 1.0), to allow reusing the concept everywhere.

And now with 1.7 we start adding new requirements.

Personally I don't think we shouldn't add pool cleanups everywhere because some code might need it.

For .Net Microsoft recommends using finalizers *only* when you have to directly release unmanaged resources like file handles. So you use them in the low layers where you are managing resources, not high up the chain where you are just wrapping lower layer.

I think we should apply the same rule here: Use pool cleanups when necessary, not by default.
We are just reinventing reference counting if we are adding pool cleanups everywhere.

The python bindings need it, so I would say: Add it there.

(And maybe make it easier to use from other bindings when it proves useful. That is why I suggested adding the function in svn_stream.h)

        Bert
Received on 2011-05-10 17:09:31 CEST