Re: Interaction between svn_error_t and bindings exceptions

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?

--dave

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org