Re: stress.pl failing over ra_svn [PATCH]

Philip Martin <philip@codematters.co.uk> writes:

> The problem occurs when a second commit starts while the first commit
> is in progress, and both commits modify the same file. This causes
> svn_repos_fs_commit_txn to return SVN_ERR_FS_CONFLICT, which
> propagates to svn_ra_svn_handle_commands where it gets written using
> svn_ra_svn_write_cmd_failure. So far so good.
>
> Now the commit function in svnserve/serve.c calls the function
> svn_ra_svn_drive_editor with pass_through set FALSE so the
> SVN_ERR_FS_CONFLICT error does not get returned. The commit function
> therefore goes on to call svn_ra_svn_write_tuple with unitialized
> new_rev, date, and author which causes svnserve to dump core.
>
> This core dump happens before svn_ra_svn_flush gets a chance to flush
> the SVN_ERR_FS_CONFLICT error to the client. This is what causes the
> client to report the network connection as closed.

I am using the following patch which lets me run stress.pl over
ra_svn. However I am not altogether happy about the way it drops all
the errors in svnserve/serve.c:commit. Perhaps "acceptable" and
"unacceptable" errors need to be distinguished, with only the former
being dropped?

Fix a bug where an SVN_ERR_FS_CONFLICT error raised during commit did
not get back to the client.

* subversion/libsvn_ra_svn/marshal.c (svn_ra_svn_handle_commands): Flush
  after writing a failure response. Don't ignore errors.

* subversion/svnserve/serve.c (commit): Only write the rev/date/author
  if the commit completed without error.

Index: ../svn/subversion/libsvn_ra_svn/marshal.c
===================================================================
--- ../svn/subversion/libsvn_ra_svn/marshal.c (revision 5483)
+++ ../svn/subversion/libsvn_ra_svn/marshal.c (working copy)
@@ -643,9 +643,17 @@
                                 "Unknown command '%s'", cmdname);
       if (err)
         {
- svn_ra_svn_write_cmd_failure(conn, subpool, err);
+ svn_error_t *err2 = svn_ra_svn_write_cmd_failure(conn, subpool, err);
+ if (! err2)
+ err2 = svn_ra_svn_flush (conn, subpool);
           if (pass_through_errors)
- return err;
+ {
+ svn_error_clear (err2);
+ return err;
+ }
+ svn_error_clear (err);
+ if (err2)
+ return err2;
         }
       svn_error_clear(err);
       apr_pool_clear(subpool);
Index: ../svn/subversion/svnserve/serve.c
===================================================================
--- ../svn/subversion/svnserve/serve.c (revision 5483)
+++ ../svn/subversion/svnserve/serve.c (working copy)
@@ -352,6 +352,7 @@
   svn_boolean_t aborted;
   commit_callback_baton_t ccb;
   svn_revnum_t new_rev;
+ svn_error_t *err;
 
   SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c", &log_msg));
   SVN_CMD_ERR(must_not_be_read_only(b));
@@ -362,10 +363,12 @@
                                           b->repos_url, b->fs_path, b->user,
                                           log_msg, commit_done, &ccb, pool));
   SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
- SVN_ERR(svn_ra_svn_drive_editor(conn, pool, editor, edit_baton, FALSE,
- &aborted));
- SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "r(?c)(?c)",
- new_rev, date, author));
+ err = svn_ra_svn_drive_editor(conn, pool, editor, edit_baton, TRUE, &aborted);
+ if (err)
+ svn_error_clear (err); /* ### Should some errrors be returned? */
+ else
+ SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "r(?c)(?c)",
+ new_rev, date, author));
   return SVN_NO_ERROR;
 }
 

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org