[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

[PATCH] Fix abort_edit in swig-py

From: Eric Gillespie <epg_at_pretzelnet.org>
Date: Wed, 02 Apr 2008 12:58:23 -0700

Without my first two patches ("Cancellation editor discards
abort_edit" and "Call abort_edit upon editor error in repos
reporter"), this test fails because abort_edit is never called:

FAIL: test_do_diff2_abort_edit (__main__.SubversionRepositoryAccessTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "subversion/bindings/swig/python/tests/ra.py", line 268, in test_do_diff2_abort_edit
    self.assertEqual(editor.aborted, True)
AssertionError: False != True

Applying only the first doesn't change anything in this case.
After applying only the second patch, the reporter calls the
default editor's abort_edit.

After applying both, the reporter calls my abort_edit, but then
we get a new error:

ERROR: test_do_diff2_abort_edit (__main__.SubversionRepositoryAccessTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "subversion/bindings/swig/python/tests/ra.py", line 265, in test_do_diff2
_abort_edit
    self.assertRaises(TestException, reporter.finish_report, reporter_baton)
  File "/usr/lib/python2.4/unittest.py", line 320, in failUnlessRaises
    callableObj(*args, **kwargs)
  File "/home/epg/work/svn/subversion2/subversion/bindings/swig/python/libsvn/ra
.py", line 212, in finish_report
    return svn_ra_reporter2_invoke_finish_report(self, *args)
  File "/home/epg/work/svn/subversion2/subversion/bindings/swig/python/libsvn/ra
.py", line 1212, in svn_ra_reporter2_invoke_finish_report
    return apply(_ra.svn_ra_reporter2_invoke_finish_report, args)
SystemError: NULL result without error in PyObject_Call

What happens here is complicated:

1. reporter.c:finish_report gets an error
   (SVN_ERR_SWIG_PY_EXCEPTION_SET, from my apply_textdelta
   throwing an exception) and calls abort_edit.

2. abort_edit has its own exceptions to deal with (as real
   abort_edit implementations do) *thereby clearing out the
   earlier Python exception from apply_textdelta*.

3. reporter.c:finish_report discards any error from abort_edit
   and returns the SVN_ERR_SWIG_PY_EXCEPTION_SET error it got in
   step 1.

4. The swig-generated wrapper _wrap_svn_ra_reporter3_invoke_finish_report
   gets the SVN_ERR_SWIG_PY_EXCEPTION_SET error and returns NULL,
   knowing that an exception is there.

Except it isn't! It was clobbered at step 2. So Python sees a
NULL return without an exception, and raises the above SystemError.

Whew!

[[[
Preserve exception state in the Python binding of the abort_edit editor
interface, so that when finish_report returns NULL the exception is still
there, rather than a 'SystemError: NULL result without error in
PyObject_Call' error.

* subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
  (abort_edit): This needs to be different from all the other editor
    callback wrappers that use close_baton, so copy a simplified version
    of that in here. But, take care to preserve any exception that may
    have already been raised (likely the reason abort_edit is being called
    in the first place).

* subversion/bindings/swig/python/tests/ra.py
  (test_do_diff2_abort_edit): Add test that raising an exception in an
    editor function causes abort_edit to be called, and that abort_edit
    can handle exceptions of its own without clobbering the original.
]]]

Index: subversion/bindings/swig/python/tests/ra.py
===================================================================
--- subversion/bindings/swig/python/tests/ra.py (revision 30158)
+++ subversion/bindings/swig/python/tests/ra.py (working copy)
@@ -234,6 +234,39 @@ class SubversionRepositoryAccessTestCase(unittest.
     self.assertEqual("A test.\n", editor.textdeltas[0].new_data)
     self.assertEqual(1, len(editor.textdeltas))
 
+ def test_do_diff2_abort_edit(self):
+ class TestException(Exception):
+ pass
+ class ChangeReceiver(delta.Editor):
+ def __init__(self):
+ self.aborted = False
+ def apply_textdelta(self, file_baton, base_checksum):
+ raise TestException
+ def abort_edit(self, pool=None):
+ try:
+ raise Exception
+ except:
+ pass
+ self.aborted = True
+ editor = ChangeReceiver()
+
+ e_ptr, e_baton = delta.make_editor(editor)
+
+ fs_revnum = fs.youngest_rev(self.fs)
+
+ sess_url = ra.get_session_url(self.ra_ctx)
+ try:
+ ra.reparent(self.ra_ctx, REPOS_URL+"/trunk")
+ reporter, reporter_baton = ra.do_diff2(self.ra_ctx, fs_revnum,
+ "README.txt", 0, 0, 1,
+ REPOS_URL+"/trunk/README.txt",
+ e_ptr, e_baton)
+ reporter.set_path(reporter_baton, "", 0, True, None)
+ self.assertRaises(TestException, reporter.finish_report, reporter_baton)
+ finally:
+ ra.reparent(self.ra_ctx, sess_url)
+ self.assertEqual(editor.aborted, True)
+
   def test_get_locations(self):
     locations = ra.get_locations(self.ra_ctx, "trunk/README.txt", 2, range(1,5))
     self.assertEqual(locations, {
Index: subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c
===================================================================
--- subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (revision 30158)
+++ subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c (working copy)
@@ -1808,7 +1808,49 @@ static svn_error_t *close_edit(void *edit_baton,
 static svn_error_t *abort_edit(void *edit_baton,
                                apr_pool_t *pool)
 {
- return close_baton(edit_baton, "abort_edit");
+ item_baton *ib = edit_baton;
+ PyObject *result;
+ svn_error_t *err;
+ PyObject *type, *value, *traceback;
+
+ svn_swig_py_acquire_py_lock();
+
+ /* Save exception state. */
+ PyErr_Fetch(&type, &value, &traceback);
+
+ /* Call abort_edit callback. */
+ result = PyObject_CallMethod(ib->editor, (char *)"abort_edit", NULL);
+ if (type == NULL && result == NULL)
+ {
+ /* We had no exception before abort and callback raised one; let the
+ callback's out. */
+ err = callback_exception_error();
+ goto finished;
+ }
+
+ /* there is no return value, so just toss this object (probably Py_None) */
+ Py_XDECREF(result);
+
+ /* We're now done with the baton. Since there isn't really a free, all
+ we need to do is note that its objects are no longer referenced by
+ the baton. */
+ Py_XDECREF(ib->baton);
+
+#ifdef SVN_DEBUG
+ ib->editor = ib->baton = NULL;
+#endif
+
+ if (type != NULL)
+ {
+ /* We had an exception before abort; discard any exception the
+ callback may have raised and restore the saved exception. */
+ PyErr_Restore(type, value, traceback);
+ }
+ err = SVN_NO_ERROR;
+
+ finished:
+ svn_swig_py_release_py_lock();
+ return err;
 }
 
 void svn_swig_py_make_editor(const svn_delta_editor_t **editor,

---------------------------------------------------------------------
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-02 21:58:34 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.