Interaction between svn_error_t and bindings exceptions

Release note: this has been broken for long enough, it need not
hold up 1.5.

Background
==========

In Python (and Ruby and Perl (and ...?)) we have exceptions. In
Subversion we have svn_error_t. When Subversion functions return
an error object, we convert that into the exception system. That
stuff is simple and we need not worry about it.

The tricky bit is callbacks. This is Python calling swig-py
calling Subversion calling swig-py calling Python:

reporter.finish_report (Python)
-> _wrap_svn_ra_reporter3_invoke_finish_report (swig-py)
   -> finish_report (Subversion repos/reporter.c)
      -> sw_apply_textdelta (swig-py swigutil_py.c)
         -> py_apply_textdelta (Python)

(If you're trying to follow along looking at the source files,
note that both sw_apply_textdelta and py_apply_textdelta are both
called 'apply_textdelta'; I mangle them here for clarity.)

What happens when a Python callback raises an exception?
- sw_apply_textdelta (and all the other callback wrappers) notices
  the exception and creates and returns a new svn_error_t with
  apr_err set to SVN_ERR_SWIG_PY_EXCEPTION_SET.
- finish_report passes it along via SVN_ERR()
- _wrap_svn_ra_reporter3_invoke_finish_report (and all other
  Subversion wrappers) check the svn_error_t return. If
  SVN_NO_ERROR, no problem; if not SVN_ERR_SWIG_PY_EXCEPTION_SET,
  create an exception and raise that (see first Background paragraph).
  But, if SVN_ERR_SWIG_PY_EXCEPTION_SET, clear the error and
  return NULL, knowing that Python has py_apply_textdelta's exception.
- reporter.finish_report: we're back in Python, and the exception
  propagages up the stack until a matching except block

Parallel errors
===============

That's all well and good until we consider the problem of
handling exceptions while an exception is already in progress.
Consider two examples, first pure Python, then pure C:

Pure Python:

  def Cleanup():
      raise CleanupError
  def DoSomething():
      try:
          ...
      except:
          exc_type, exc_val, exc_tb = sys.exc_info()
          try:
              Cleanup()
          except:
              pass
          raise exc_type, exc_val, exc_tb

Pure C:
  Cleanup() {
      return svn_error_t("abort error");
  }
  DoSomething() {
      svn_error_t *err = ...;
      if (err != SVN_NO_ERROR)
          svn_error_clear(Cleanup());
      return err;
  }

Simple enough. See the end of repos/reporter.c:finish_report for
this pattern in action.

The problem
===========

But what happens if DoSomething is C and Cleanup is Python?
Don't cross the streams! Replace DoSomething and Cleanup with
finish_report and our Python abort_edit from the Background
example.

finish_report clears the SVN_ERR_SWIG_PY_EXCEPTION_SET
svn_error_t but it doesn't touch the Python interpreter's
exception state! Our exception/svn_error_t mapping is
asymmetrical; we map the Python exception into an svn_error_t,
but we don't map the clearing of an svn_error_t back to a
clearing of the Python exception state:

Pure Subversion:

my_apply_textdelta()
{
    return svn_error_t("delta error");
}
my_abort_edit()
{
    return svn_error_t("abort error");
}
my_diff() {
    ...
    err = reporter->finish_report();
    // err is the the "delta error", not "abort error"
    // the reporter discarded the "abort error"
}

Python calling swig-py calling Subversion calling swig-py calling Python:

def my_apply_textdelta():
    raise Exception("delta error")
def my_abort_edit():
    raise Exception("abort error")
def my_diff():
    ...
    try:
        reporter.finish_report()
    except Exception, err:
        # err is the the "abort error", not "delta error"
        # the reporter discarded the SVN_ERR_SWIG_PY_EXCEPTION_SET svn_error_t
        # but DID NOT clear the Python exception!

Solutions
=========

Somehow the svn_error_clear at the end of reporter.c:
finish_report has to trigger clearing the Python exception.

All solutions involve modifying at least
svn_types.swg:%typemap(out) svn_error_t * and
swigutil_py.c:callback_exception_error .

- Extend svn_error_t with bindings callback/baton

  svn_error_t constructors set this to NULL, and bindings may
  then fill it in:

  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");
    PyErr_Fetch(&type, &value, &traceback);
    err->ffi_func = callback_exception_error_cb;
    err->ffi_baton = Tuple(type, value, traceback);
  }

  %typemap(out) svn_error_t * {
    if ($1 != NULL) {
        if ($1->apr_err == SVN_ERR_SWIG_PY_EXCEPTION_SET) {
            (type, value, traceback) = $1->ffi_baton;
            $1->ffi_func = NULL;
            if (type != NULL)
              PyErr_Restore(type, value, traceback);
            svn_error_clear($1);
        } else
            svn_swig_py_svn_exception($1);
        SWIG_fail;
    }
    Py_INCREF(Py_None);
    $result = Py_None;
  }

  void
  svn_error_clear(svn_error_t *err) {
    if (err->ffi_callback != NULL)
      err->ffi_callback(err->ffi_baton);
    ...
  }

  In theory, we can extend even non-opaque structures at the end
  as long as we require that callers always use a constructor.
  This seems to be the case for svn_error_t, so revving would not
  be necessary. Glasser and kfogel seem fine with this, but epg
  is quite wary.

- Variant: rev svn_error_t

  Dodges potential (or imaginary) problems with extending an
  existing structure, but gives everyone involved a heart attack.
  If there's any type we never want to rev, this is the one...

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

- Global dict mapping svn_error_t to exception state

  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");
    PyErr_Fetch(&type, &value, &traceback);
    Global_Err_Map[err] = (type, value, traceback)
  }

  %typemap(out) svn_error_t * {
    if ($1 != NULL) {
        if ($1->apr_err == SVN_ERR_SWIG_PY_EXCEPTION_SET) {
            (type, value, traceback) = Global_Err_Map[$1]
            del Global_Err_Map[$1]
            if (type != NULL)
              PyErr_Restore(type, value, traceback);
            svn_error_clear($1);
        } else
            svn_swig_py_svn_exception($1);
        SWIG_fail;
    }
    Py_INCREF(Py_None);
    $result = Py_None;
  }

  I'm not sure how to create the global dict thread-safely.
  Worse, the exception state is leaked when one of the error from
  abort_edit is discarded.

- 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()).

-- 
Eric Gillespie <*> epg_at_pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org