You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2015/03/03 02:06:16 UTC
svn commit: r1663500 - in /subversion/trunk/subversion:
libsvn_ra_serf/commit.c libsvn_ra_serf/merge.c libsvn_ra_serf/ra_serf.h
tests/cmdline/lock_tests.py
Author: rhuijben
Date: Tue Mar 3 01:06:16 2015
New Revision: 1663500
URL: http://svn.apache.org/r1663500
Log:
In ra-serf (for issue #4557) reinstate a bit of code that retries a delete
with an altered request if the original request fails, because the server
determined that it is an invalid request, because it has too many bytes
in the headers.
Before r1553501 we always retried DELETE requests that required lock
tokens, as the initial request didn't have an If header at all.
* subversion/libsvn_ra_serf/commit.c
(delete_context_t): Add boolean.
(setup_delete_headers): Only setup a single If when recursive lock
headers are disabled.
(create_delete_body): New function, based on the version removed in
r1553501.
(delete_entry): Detect a failed request and run a retry if necessary.
(add_directory): Document limitation.
* subversion/libsvn_ra_serf/merge.c
(merge_lock_token_list): Rename back to...
(svn_ra_serf__merge_lock_token_list): ... this.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__merge_lock_token_list): New function.
* subversion/tests/cmdline/lock_tests.py
(delete_dir_with_lots_of_locked_files): Extend test to show
that the MKCOL case isn't fixed.
Modified:
subversion/trunk/subversion/libsvn_ra_serf/commit.c
subversion/trunk/subversion/libsvn_ra_serf/merge.c
subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
subversion/trunk/subversion/tests/cmdline/lock_tests.py
Modified: subversion/trunk/subversion/libsvn_ra_serf/commit.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/commit.c?rev=1663500&r1=1663499&r2=1663500&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/commit.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/commit.c Tue Mar 3 01:06:16 2015
@@ -101,6 +101,8 @@ typedef struct delete_context_t {
svn_revnum_t revision;
commit_context_t *commit_ctx;
+
+ svn_boolean_t non_recursive_if; /* Only create a non-recursive If header */
} delete_context_t;
/* Represents a directory. */
@@ -1101,8 +1103,15 @@ setup_delete_headers(serf_bucket_t *head
serf_bucket_headers_set(headers, SVN_DAV_VERSION_NAME_HEADER,
apr_ltoa(pool, del->revision));
- SVN_ERR(setup_if_header_recursive(&added, headers, del->commit_ctx,
- del->relpath, pool));
+ if (! del->non_recursive_if)
+ SVN_ERR(setup_if_header_recursive(&added, headers, del->commit_ctx,
+ del->relpath, pool));
+ else
+ {
+ SVN_ERR(maybe_set_lock_token_header(headers, del->commit_ctx,
+ del->relpath, pool));
+ added = TRUE;
+ }
if (added && del->commit_ctx->keep_locks)
serf_bucket_headers_setn(headers, SVN_DAV_OPTIONS_HEADER,
@@ -1402,6 +1411,28 @@ open_root(void *edit_baton,
return SVN_NO_ERROR;
}
+/* Implements svn_ra_serf__request_body_delegate_t */
+static svn_error_t *
+create_delete_body(serf_bucket_t **body_bkt,
+ void *baton,
+ serf_bucket_alloc_t *alloc,
+ apr_pool_t *pool /* request pool */,
+ apr_pool_t *scratch_pool)
+{
+ delete_context_t *ctx = baton;
+ serf_bucket_t *body;
+
+ body = serf_bucket_aggregate_create(alloc);
+
+ svn_ra_serf__add_xml_header_buckets(body, alloc);
+
+ svn_ra_serf__merge_lock_token_list(ctx->commit_ctx->lock_tokens,
+ ctx->relpath, body, alloc, pool);
+
+ *body_bkt = body;
+ return SVN_NO_ERROR;
+}
+
static svn_error_t *
delete_entry(const char *path,
svn_revnum_t revision,
@@ -1412,6 +1443,7 @@ delete_entry(const char *path,
delete_context_t *delete_ctx;
svn_ra_serf__handler_t *handler;
const char *delete_target;
+ svn_error_t *err;
if (USING_HTTPV2_COMMIT_SUPPORT(dir->commit_ctx))
{
@@ -1446,7 +1478,23 @@ delete_entry(const char *path,
handler->method = "DELETE";
handler->path = delete_target;
- SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
+ err = svn_ra_serf__context_run_one(handler, pool);
+ if (err && err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED
+ && handler->sline.code == 400)
+ {
+ svn_error_clear(err);
+
+ /* Try again with non-standard body to overcome Apache Httpd
+ header limit */
+ delete_ctx->non_recursive_if = TRUE;
+ handler->body_type = "text/xml";
+ handler->body_delegate = create_delete_body;
+ handler->body_delegate_baton = delete_ctx;
+
+ SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
+ }
+ else
+ SVN_ERR(err);
/* 204 No Content: item successfully deleted */
if (handler->sline.code != 204)
@@ -1539,7 +1587,9 @@ add_directory(const char *path,
handler->header_delegate = setup_copy_dir_headers;
handler->header_delegate_baton = dir;
}
-
+ /* We have the same problem as with DELETE here: if there are too many
+ locks, the request fails. But in this case there is no way to retry
+ with a non-standard request. #### How to fix? */
SVN_ERR(svn_ra_serf__context_run_one(handler, dir->pool));
if (handler->sline.code != 201)
Modified: subversion/trunk/subversion/libsvn_ra_serf/merge.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/merge.c?rev=1663500&r1=1663499&r2=1663500&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/merge.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/merge.c Tue Mar 3 01:06:16 2015
@@ -285,12 +285,12 @@ setup_merge_headers(serf_bucket_t *heade
return SVN_NO_ERROR;
}
-static void
-merge_lock_token_list(apr_hash_t *lock_tokens,
- const char *parent,
- serf_bucket_t *body,
- serf_bucket_alloc_t *alloc,
- apr_pool_t *pool)
+void
+svn_ra_serf__merge_lock_token_list(apr_hash_t *lock_tokens,
+ const char *parent,
+ serf_bucket_t *body,
+ serf_bucket_alloc_t *alloc,
+ apr_pool_t *pool)
{
apr_hash_index_t *hi;
@@ -378,7 +378,8 @@ create_merge_body(serf_bucket_t **bkt,
"D:creator-displayname", SVN_VA_NULL);
svn_ra_serf__add_close_tag_buckets(body_bkt, alloc, "D:prop");
- merge_lock_token_list(ctx->lock_tokens, NULL, body_bkt, alloc, pool);
+ svn_ra_serf__merge_lock_token_list(ctx->lock_tokens, NULL, body_bkt,
+ alloc, pool);
svn_ra_serf__add_close_tag_buckets(body_bkt, alloc, "D:merge");
Modified: subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h?rev=1663500&r1=1663499&r2=1663500&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Tue Mar 3 01:06:16 2015
@@ -1021,6 +1021,13 @@ svn_ra_serf__svnname_from_wirename(const
/** MERGE-related functions **/
+void
+svn_ra_serf__merge_lock_token_list(apr_hash_t *lock_tokens,
+ const char *parent,
+ serf_bucket_t *body,
+ serf_bucket_alloc_t *alloc,
+ apr_pool_t *pool);
+
/* Create an MERGE request aimed at the SESSION url, requesting the
merge of the resource identified by MERGE_RESOURCE_URL.
LOCK_TOKENS is a hash mapping paths to lock tokens owned by the
Modified: subversion/trunk/subversion/tests/cmdline/lock_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/lock_tests.py?rev=1663500&r1=1663499&r2=1663500&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/lock_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/lock_tests.py Tue Mar 3 01:06:16 2015
@@ -2381,9 +2381,16 @@ def delete_dir_with_lots_of_locked_files
svntest.actions.run_and_verify_svn(None, [], 'lock',
'-m', 'All locks',
*locked_paths)
- # Locally delete A
+ # Locally delete A (regression against earlier versions, which
+ # always used a special non-standard request)
sbox.simple_rm("A")
+ # But a further replacement never worked
+ sbox.simple_mkdir("A")
+ # And an additional propset didn't work either
+ # (but doesn't require all lock tokens recursively)
+ sbox.simple_propset("k", "v", "A")
+
# Commit the deletion
# XFAIL: As of 1.8.10, this commit fails with:
# svn: E175002: Unexpected HTTP status 400 'Bad Request' on '<path>'
Re: svn commit: r1663500 - in /subversion/trunk/subversion:
libsvn_ra_serf/commit.c libsvn_ra_serf/merge.c libsvn_ra_serf/ra_serf.h tests/cmdline/lock_tests.py
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 12 April 2016 at 10:30, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 3 March 2015 at 04:06, <rh...@apache.org> wrote:
>> Author: rhuijben
>> Date: Tue Mar 3 01:06:16 2015
>> New Revision: 1663500
>>
>> URL: http://svn.apache.org/r1663500
>> Log:
>> In ra-serf (for issue #4557) reinstate a bit of code that retries a delete
>> with an altered request if the original request fails, because the server
>> determined that it is an invalid request, because it has too many bytes
>> in the headers.
>>
>> Before r1553501 we always retried DELETE requests that required lock
>> tokens, as the initial request didn't have an If header at all.
>>
> Hi Bert,
>
> See my review inline.
>
> [...]
>
>> Modified: subversion/trunk/subversion/libsvn_ra_serf/commit.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/commit.c?rev=1663500&r1=1663499&r2=1663500&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_ra_serf/commit.c (original)
>> +++ subversion/trunk/subversion/libsvn_ra_serf/commit.c Tue Mar 3 01:06:16 2015
> [...]
>
>> static svn_error_t *
>> delete_entry(const char *path,
>> svn_revnum_t revision,
>> @@ -1412,6 +1443,7 @@ delete_entry(const char *path,
>> delete_context_t *delete_ctx;
>> svn_ra_serf__handler_t *handler;
>> const char *delete_target;
>> + svn_error_t *err;
>>
>> if (USING_HTTPV2_COMMIT_SUPPORT(dir->commit_ctx))
>> {
>> @@ -1446,7 +1478,23 @@ delete_entry(const char *path,
>> handler->method = "DELETE";
>> handler->path = delete_target;
>>
>> - SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
>> + err = svn_ra_serf__context_run_one(handler, pool);
>> + if (err && err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED
>> + && handler->sline.code == 400)
> May be it's better set handler->no_fail_on_http_failure_status to TRUE
> and check for handler->sline.code? This will remove dependency on
> specific error code and simplify code a bit.
>
>> + {
>> + svn_error_clear(err);
>> +
>> + /* Try again with non-standard body to overcome Apache Httpd
>> + header limit */
>> + delete_ctx->non_recursive_if = TRUE;
>> + handler->body_type = "text/xml";
>> + handler->body_delegate = create_delete_body;
>> + handler->body_delegate_baton = delete_ctx;
>> +
>> + SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
> I'm not sure that we can reuse svn_ra_serf__handler_t instance for
> multiple requests.
>
> I propose the following patch. Do you have any objections?
>
Committed in r1739278 and r1739280. And added them to r1663500
nomination in 1.9.x.
--
Ivan Zhakov
Re: svn commit: r1663500 - in /subversion/trunk/subversion:
libsvn_ra_serf/commit.c libsvn_ra_serf/merge.c libsvn_ra_serf/ra_serf.h tests/cmdline/lock_tests.py
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 3 March 2015 at 04:06, <rh...@apache.org> wrote:
> Author: rhuijben
> Date: Tue Mar 3 01:06:16 2015
> New Revision: 1663500
>
> URL: http://svn.apache.org/r1663500
> Log:
> In ra-serf (for issue #4557) reinstate a bit of code that retries a delete
> with an altered request if the original request fails, because the server
> determined that it is an invalid request, because it has too many bytes
> in the headers.
>
> Before r1553501 we always retried DELETE requests that required lock
> tokens, as the initial request didn't have an If header at all.
>
Hi Bert,
See my review inline.
[...]
> Modified: subversion/trunk/subversion/libsvn_ra_serf/commit.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/commit.c?rev=1663500&r1=1663499&r2=1663500&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/commit.c Tue Mar 3 01:06:16 2015
[...]
> static svn_error_t *
> delete_entry(const char *path,
> svn_revnum_t revision,
> @@ -1412,6 +1443,7 @@ delete_entry(const char *path,
> delete_context_t *delete_ctx;
> svn_ra_serf__handler_t *handler;
> const char *delete_target;
> + svn_error_t *err;
>
> if (USING_HTTPV2_COMMIT_SUPPORT(dir->commit_ctx))
> {
> @@ -1446,7 +1478,23 @@ delete_entry(const char *path,
> handler->method = "DELETE";
> handler->path = delete_target;
>
> - SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
> + err = svn_ra_serf__context_run_one(handler, pool);
> + if (err && err->apr_err == SVN_ERR_RA_DAV_REQUEST_FAILED
> + && handler->sline.code == 400)
May be it's better set handler->no_fail_on_http_failure_status to TRUE
and check for handler->sline.code? This will remove dependency on
specific error code and simplify code a bit.
> + {
> + svn_error_clear(err);
> +
> + /* Try again with non-standard body to overcome Apache Httpd
> + header limit */
> + delete_ctx->non_recursive_if = TRUE;
> + handler->body_type = "text/xml";
> + handler->body_delegate = create_delete_body;
> + handler->body_delegate_baton = delete_ctx;
> +
> + SVN_ERR(svn_ra_serf__context_run_one(handler, pool));
I'm not sure that we can reuse svn_ra_serf__handler_t instance for
multiple requests.
I propose the following patch. Do you have any objections?
--
Ivan Zhakov