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.
>From 21891860ef4dea0d8bca017314d1e9a7e843da3f Mon Sep 17 00:00:00 2001
From: Nick Piper <nick.pi...@logica.com>
Date: Mon, 9 May 2011 15:20:13 +0100
Subject: [PATCH] Add svn_stream_set_cleanup(), which can be used to register cleanup function
This cleanup function will be called when the pool containing the stream is being cleared, or when the stream is closed normally.
This is particularly needed for the Python bindings. The PyObject wrapped by the svn_stream_t created by svn_swig_py_make_stream() might go scope in Python, but the stream maintains a reference to it so the PyObject doesn't get destroyed/deleted.
---
.../swig/python/libsvn_swig_py/swigutil_py.c | 4 +-
subversion/include/svn_io.h | 11 ++++
subversion/libsvn_subr/stream.c | 58 ++++++++++++++++----
3 files changed, 61 insertions(+), 12 deletions(-)
diff --git a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
index d8b9c08..2bdc3fb 100644
--- a/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
+++ b/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
@@ -2105,7 +2105,7 @@ write_handler_pyio(void *baton, const char *data, apr_size_t *len)
}
static svn_error_t *
-close_handler_pyio(void *baton)
+cleanup_handler_pyio(void *baton)
{
PyObject *py_io = baton;
svn_swig_py_acquire_py_lock();
@@ -2122,8 +2122,8 @@ svn_swig_py_make_stream(PyObject *py_io, apr_pool_t *pool)
stream = svn_stream_create(py_io, pool);
svn_stream_set_read(stream, read_handler_pyio);
svn_stream_set_write(stream, write_handler_pyio);
- svn_stream_set_close(stream, close_handler_pyio);
Py_INCREF(py_io);
+ svn_stream_set_cleanup(stream, cleanup_handler_pyio);
return stream;
}
diff --git a/subversion/include/svn_io.h b/subversion/include/svn_io.h
index c518d24..1f56018 100644
--- a/subversion/include/svn_io.h
+++ b/subversion/include/svn_io.h
@@ -763,6 +763,9 @@ typedef svn_error_t *(*svn_write_fn_t)(void *baton,
/** Close handler function for a generic stream. @see svn_stream_t. */
typedef svn_error_t *(*svn_close_fn_t)(void *baton);
+/** Cleanup handler function for a generic stream. @see svn_stream_t. */
+typedef svn_error_t *(*svn_cleanup_fn_t)(void *baton);
+
/** An opaque type which represents a mark on a stream. There is no
* concrete definition of this type, it is a named type for stream
* implementation specific baton pointers.
@@ -829,6 +832,14 @@ void
svn_stream_set_close(svn_stream_t *stream,
svn_close_fn_t close_fn);
+/** Set @a stream's cleanup function to @a close_fn
+ *
+ * @since New in 1.7.
+ */
+void
+svn_stream_set_cleanup(svn_stream_t *stream,
+ svn_cleanup_fn_t cleanup_fn);
+
/** Set @a stream's mark function to @a mark_fn
*
* @since New in 1.7.
diff --git a/subversion/libsvn_subr/stream.c b/subversion/libsvn_subr/stream.c
index f03f963..16497a8 100644
--- a/subversion/libsvn_subr/stream.c
+++ b/subversion/libsvn_subr/stream.c
@@ -54,6 +54,8 @@ struct svn_stream_t {
svn_io_mark_fn_t mark_fn;
svn_io_seek_fn_t seek_fn;
svn_io_buffered_fn_t buffered_fn;
+ svn_cleanup_fn_t cleanup_fn;
+ apr_pool_t *pool; /* to allow us to deregister clean up functions */
};
@@ -73,9 +75,45 @@ svn_stream_create(void *baton, apr_pool_t *pool)
stream->mark_fn = NULL;
stream->seek_fn = NULL;
stream->buffered_fn = NULL;
+ stream->cleanup_fn = NULL;
+ stream->pool = pool;
return stream;
}
+/*
+ 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.
+ */
+static apr_status_t
+stream_cleanup(svn_stream_t *stream)
+{
+ if (stream->cleanup_fn != NULL)
+ {
+ apr_pool_cleanup_kill(stream->pool, stream, (apr_status_t(*)(void *)) stream_cleanup);
+ stream->cleanup_fn(stream->baton); // how to handle returning error?
+ }
+ return APR_SUCCESS;
+}
+
+svn_error_t *
+svn_stream_close(svn_stream_t *stream)
+{
+ apr_status_t apr_status;
+ svn_error_t *err1 = SVN_NO_ERROR;
+ svn_error_t *err2 = SVN_NO_ERROR;
+ if (stream->close_fn != NULL)
+ err1 = stream->close_fn(stream->baton);
+ apr_status = stream_cleanup(stream);
+ if (apr_status != APR_SUCCESS)
+ err2 = svn_error_wrap_apr(apr_status, "Stream cleanup function failed");
+ return svn_error_compose_create(err1, err2);
+}
+
void
svn_stream_set_baton(svn_stream_t *stream, void *baton)
@@ -109,6 +146,16 @@ svn_stream_set_close(svn_stream_t *stream, svn_close_fn_t close_fn)
}
void
+svn_stream_set_cleanup(svn_stream_t *stream, svn_cleanup_fn_t cleanup_fn)
+{
+ stream->cleanup_fn = cleanup_fn;
+ apr_pool_cleanup_register(stream->pool, stream,
+ (apr_status_t(*)(void *)) stream_cleanup,
+ apr_pool_cleanup_null);
+
+}
+
+void
svn_stream_set_mark(svn_stream_t *stream, svn_io_mark_fn_t mark_fn)
{
stream->mark_fn = mark_fn;
@@ -193,15 +240,6 @@ svn_stream_buffered(svn_stream_t *stream)
}
svn_error_t *
-svn_stream_close(svn_stream_t *stream)
-{
- if (stream->close_fn == NULL)
- return SVN_NO_ERROR;
- return stream->close_fn(stream->baton);
-}
-
-
-svn_error_t *
svn_stream_printf(svn_stream_t *stream,
apr_pool_t *pool,
const char *fmt,
--
1.7.1
