On Fri, Apr 4, 2008 at 10:48 AM, David Glasser <glasser_at_davidglasser.net> wrote:
>
> On Thu, Apr 3, 2008 at 9:53 PM, David Glasser <glasser_at_davidglasser.net> wrote:
> >
> > On Thu, Apr 3, 2008 at 7:06 PM, Eric Gillespie <epg_at_pretzelnet.org> wrote:
> > > - Provide an svn_error_t lookalike in swig-py
> > >
> > > struct swig_py_svn_error_t {
> > > struct svn_error_t err;
> > > PyObject *type, *value, *traceback;
> > > }
> > > static void *callback_exception_error_cb(void *baton) {
> > > PyObject (*type, *value, *traceback) = baton;
> > > Py_XDECREF them all
> > > }
> > > static svn_error_t *callback_exception_error(void)
> > > {
> > > PyObject *type, *value, *traceback;
> > > svn_error_t *err = svn_error_create(SVN_ERR_SWIG_PY_EXCEPTION_SET, NULL,
> > > "Python callback raised an exception");
> > > swig_py_svn_error_t *perr = swig_py_svn_error_t();
> > > copy err fields over to perr;
> > > PyErr_Fetch(&perr->type, &perr->value, &perr->traceback);
> > > register a cleanup handler for callback_exception_error_cb on the err pool;
> > > }
> > >
> > > %typemap(out) svn_error_t * {
> > > if ($1 != NULL) {
> > > if ($1->apr_err == SVN_ERR_SWIG_PY_EXCEPTION_SET) {
> > > unregister pool cleanup handler;
> > > if ((py_err)$1->type != NULL)
> > > PyErr_Restore(py_err)$1->type, py_err)$1->value,
> > > py_err)$1->traceback);
> > > svn_error_clear($1);
> > > } else
> > > svn_swig_py_svn_exception($1);
> > > SWIG_fail;
> > > }
> > > Py_INCREF(Py_None);
> > > $result = Py_None;
> > > }
> >
> > I almost completely like this one, except for worries about handling
> > crap like svn_error_dup and svn_error_compose, which are going to do a
> > poor job of copying extra data they don't know about.
> >
> >
> > > - Use pool cleanup like the lookalike solution, but with userdata
> > >
> > > This is my favorite solution, but I didn't think of it until
> > > after Glasser left, so it hasn't had the obvious reasons why it
> > > couldn't possibly work pointed out. Glasser? ;->
> > >
> > > If we're using the pool cleanup handler to free the exception
> > > objects, why not also store them as userdata on that field?
> > > Then we don't need to extend or rev any types, fake out any
> > > types, or manage a global dict (with leaks).
> > >
> > > (Side note: swigutil_py.c:callback_bad_return_error is broken,
> > > and the TypeError gets clobbered by a SubversionError for the
> > > APR_EGENERAL it returns; instead it needs to return
> > > callback_exception_error()).
> >
> > This also doesn't work very well with chained errors, I think, since
> > they share a pool (not to mention dup/compose).
> >
> >
> >
> > ... OK. So this is a bit of a hack, but it would work. We can do the
> > "bigger punned struct" thing, but keep it internal to
> > libsvn_subr/error.c.
> >
> > Define an error category SVN_ERR_WRAPPED_* in svn_error_codes.h.
> >
> > In error.c, do
> >
> > struct wrapped_error {
> >
> > struct svn_error_t err;
> > void *baton;
> > };
> >
> > #define ERR_IS_WRAPPED(x) (((x) >= SVN_ERR_WRAPPED_CATEGORY_START) &&
> > ((x) < SVN_ERR_CATEGORY_AFTER_WRAPPED_START))
> > #define ERR_STRUCT_SIZE(x) (ERR_IS_WRAPPED(x) ? sizeof(struct
> > wrapped_error) : sizeof(struct svn_error_t))
> >
> > Everywhere in error.c that allocates an error (make_error_internal,
> > svn_error_compose, svn_error_dup) would use ERR_STRUCT_SIZE rather
> > than a hardcoded sizeof. The "*a = *b" lines in svn_error_compose and
> > svn_error_dup might need to be tweaked a bit as well.
> >
> > Finally, just add
> >
> > void *svn_error_get_wrap_baton(svn_error_t *err)
> > {
> > struct wrapped_error *w = err;
> > assert(ERR_IS_WRAPPED(err->apr_err));
> > return w->baton;
> > }
> >
> > void svn_error_set_wrap_baton(svn_error_t *err, void *baton)
> > {
> > struct wrapped_error *w = err;
> > assert(ERR_IS_WRAPPED(err->apr_err));
> > w->baton = baton;
> > }
> >
> > No memory leaks. No implementation details leaked outside error.c
> > (and documentation of some binding-related APIs). Allows you to
> > define SVN_ERR_WRAPPED_PYTHON and SVN_ERR_WRAPPED_PERL with each
> > language only unwrapping its own exceptions (and leaving the other
> > language's as opaque). Exceptions and errors never get out of touch
> > with each other.
> >
> > Finally, document explicitly that err->pool is cleared/destroyed if
> > and only if svn_error_clear or svn_handle_errorN is called, and set a
> > decrefing pool handler in Python. (For completeness, perhaps wrapped
> > errors also need callbacks that are called when svn_error_dup/compose
> > are called, since swigpy will need to tweak some refcounts there as
> > well. And while you're doing that, eh, maybe do the clear thing as a
> > callback rather than as a pool cleanup handler.
> >
> > Does this sound non-insane?
>
> A slight refinement:
>
> * Ban creating SVN_ERR_WRAPPED_* errors with the normal functions
> (svn_error_create[f], svn_error_wrap_apr).
>
> * Actually expose svn_error_bindings_wrapper_t.
>
> * Make a new svn_error_wrap_from_bindings:
>
> typedef void (svn_error_clear_callback)(svn_error_bindings_wrapper_t *err);
> typedef void (svn_error_dup_callback)(svn_error_bindings_wrapper_ *err);
>
> svn_error_t *svn_error_wrap_external_error(apr_status_t apr_err, void
> *baton, svn_error_clear_callback *clear_cb, svn_error_dup_callback
> *dup_cb)
> {
> assert(ERR_IS_WRAPPED(apr_err));
> [ make the error, with the bigger size, as above ]
> store baton, clear_cb, dup_cb in the "extra" error fields
> }
>
> The clear callback is called from svn_error_clear and
> svn_handle_error; the dup callback is called from svn_error_dup;
> svn_error_compose doesn't need to do anything. No pool callbacks,
> either.
>
> For Python, the "clear" callback does a decref and the "dup" callback
> does an incref. The "return-error-from-C-to-Python" code checks to
> see if it's SVN_ERR_WRAPPED_SWIG_PYTHON, and in that case casts to
> svn_error_bindings_wrapper_t * and pulls out the baton.
This sounds great! I like that you managed to keep all of the changes
contained so that they won't affect code other than the bindings.
The ctypes bindings have dodged this issue so far because we don't
allow Python exceptions to be thrown from inside callbacks. Instead we
require (right now) that the Python callbacks return appropriate error
codes. This also therefore dodges the requirement that the exceptions
propagate up the stack.
When we do extend the higher level ctypes Python bindings to support
catching and wrapping exceptions inside callbacks, we'll benefit from
your work here.
Cheers,
David
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-04-05 17:47:07 CEST