Re: Interaction between svn_error_t and bindings exceptions

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.

--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