Author: rhuijben
Date: Sat Feb 14 22:56:38 2015
New Revision: 1659867
URL: http://svn.apache.org/r1659867
Log:
Add a safety net for a few ugly corner case where users would go into error
handling and abort a commit that just succeeded, just because the commit
callback failed.
Daniel Shahaf reproduced this edge case by somehow causing a SIGPIPE in
the commit handler by producing a ^C at the right time. See todays irc
log for details.
ra-svn already had a similar safety net here.
* subversion/libsvn_ra_serf/commit.c
(close_edit): Make sure we don't try to abort a succeeded txn, by
setting the right variables.
* subversion/libsvn_repos/commit.c
(close_edit): Clear some variables to avoid a theoretical case where
we can abort a succeeded commit.
* subversion/tests/libsvn_ra/ra-test.c
(includes): Add svn_time.h.
(commit_callback_with_failure): New function.
(commit_callback_failure): New function.
(test_list): Add commit_callback_failure.
Modified:
subversion/trunk/subversion/libsvn_ra_serf/commit.c
subversion/trunk/subversion/libsvn_repos/commit.c
subversion/trunk/subversion/tests/libsvn_ra/ra-test.c
Modified: subversion/trunk/subversion/libsvn_ra_serf/commit.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/commit.c?rev=1659867&r1=1659866&r2=1659867&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/commit.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/commit.c Sat Feb 14 22:56:38 2015
@@ -1975,6 +1975,7 @@ close_edit(void *edit_baton,
const char *merge_target =
ctx->activity_url ? ctx->activity_url : ctx->txn_url;
const svn_commit_info_t *commit_info;
+ svn_error_t *err = NULL;
/* MERGE our activity */
SVN_ERR(svn_ra_serf__run_merge(&commit_info,
@@ -1984,9 +1985,11 @@ close_edit(void *edit_baton,
ctx->keep_locks,
pool, pool));
+ ctx->txn_url = NULL; /* If HTTPv2, the txn is now done */
+
/* Inform the WC that we did a commit. */
if (ctx->callback)
- SVN_ERR(ctx->callback(commit_info, ctx->callback_baton, pool));
+ err = ctx->callback(commit_info, ctx->callback_baton, pool);
/* If we're using activities, DELETE our completed activity. */
if (ctx->activity_url)
@@ -2003,12 +2006,16 @@ close_edit(void *edit_baton,
ctx->activity_url = NULL; /* Don't try again in abort_edit() on fail */
- SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
+ SVN_ERR(svn_error_compose_create(
+ err,
+ svn_ra_serf__context_run_one(handler, pool)));
if (handler->sline.code != 204)
return svn_error_trace(svn_ra_serf__unexpected_status(handler));
}
+ SVN_ERR(err);
+
return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/libsvn_repos/commit.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1659867&r1=1659866&r2=1659867&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/commit.c (original)
+++ subversion/trunk/subversion/libsvn_repos/commit.c Sat Feb 14 22:56:38 2015
@@ -780,6 +780,13 @@ close_edit(void *edit_baton,
post_commit_err = svn_repos__post_commit_error_str(err, pool);
svn_error_clear(err);
}
+
+ /* Make sure a future abort doesn't perform
+ any work. This may occur if the commit
+ callback returns an error! */
+
+ eb->txn = NULL;
+ eb->txn_root = NULL;
}
else
{
Modified: subversion/trunk/subversion/tests/libsvn_ra/ra-test.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_ra/ra-test.c?rev=1659867&r1=1659866&r2=1659867&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_ra/ra-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_ra/ra-test.c Sat Feb 14 22:56:38
2015
@@ -32,6 +32,7 @@
#include "svn_error.h"
#include "svn_delta.h"
#include "svn_ra.h"
+#include "svn_time.h"
#include "svn_pools.h"
#include "svn_cmdline.h"
#include "svn_dirent_uri.h"
@@ -616,6 +617,52 @@ get_dir_test(const svn_test_opts_t *opts
return SVN_NO_ERROR;
}
+/* Implements svn_commit_callback2_t for commit_callback_failure() */
+static svn_error_t *
+commit_callback_with_failure(const svn_commit_info_t *info,
+ void *baton,
+ apr_pool_t *scratch_pool)
+{
+ apr_time_t timetemp;
+
+ SVN_TEST_ASSERT(info != NULL);
+ SVN_TEST_STRING_ASSERT(info->author, "jrandom");
+ SVN_TEST_STRING_ASSERT(info->post_commit_err, NULL);
+
+ SVN_ERR(svn_time_from_cstring(&timetemp, info->date, scratch_pool));
+ SVN_TEST_ASSERT(info->date != 0);
+ SVN_TEST_ASSERT(info->repos_root != NULL);
+ SVN_TEST_ASSERT(info->revision == 1);
+
+ return svn_error_create(SVN_ERR_CANCELLED, NULL, NULL);
+}
+
+static svn_error_t *
+commit_callback_failure(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_ra_session_t *ra_session;
+ const svn_delta_editor_t *editor;
+ void *edit_baton;
+ void *root_baton;
+ SVN_ERR(make_and_open_repos(&ra_session, "commit_cb_failure", opts, pool));
+
+ SVN_ERR(svn_ra_get_commit_editor3(ra_session, &editor, &edit_baton,
+ apr_hash_make(pool),
commit_callback_with_failure,
+ NULL, NULL, FALSE, pool));
+
+ SVN_ERR(editor->open_root(edit_baton, 1, pool, &root_baton));
+ SVN_ERR(editor->change_dir_prop(root_baton, "A",
+ svn_string_create("B", pool), pool));
+ SVN_ERR(editor->close_directory(root_baton, pool));
+ SVN_TEST_ASSERT_ERROR(editor->close_edit(edit_baton, pool),
+ SVN_ERR_CANCELLED);
+
+ /* This is what users should do if close_edit fails... Except that in this
case
+ the commit actually succeeded*/
+ SVN_ERR(editor->abort_edit(edit_baton, pool));
+ return SVN_NO_ERROR;
+}
/* The test table. */
@@ -635,6 +682,8 @@ static struct svn_test_descriptor_t test
"lock multiple paths"),
SVN_TEST_OPTS_PASS(get_dir_test,
"test ra_get_dir2"),
+ SVN_TEST_OPTS_PASS(commit_callback_failure,
+ "commit callback failure"),
SVN_TEST_NULL
};
