You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by gs...@apache.org on 2012/03/19 23:04:37 UTC
svn commit: r1302682 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h sb_bucket.c serf.c update.c util.c
Author: gstein
Date: Mon Mar 19 22:04:36 2012
New Revision: 1302682
URL: http://svn.apache.org/viewvc?rev=1302682&view=rev
Log:
Work on issue 3979: don't try to send chunked requests to HTTP/1.0
servers. This will require serf 1.1.x and a new API.
This commit lays all the groundwork, but doesn't change behavior yet.
When HTTP/1.0 requests are needed, the request body will be placed
into a spillbuf and then we use a new bucket type for the body which
reads content out of the spillbuf. Meanwhile, the spillbuf gives us
the total body size which we can place into a Content-Length header.
* subversion/libsvn_ra_serf/ra_serf.h:
(svn_ra_serf__connection_t): add an http10 flag indicating that we
must speak HTTP/1.0 to the server. it will be initially set, and
then cleared when we learn the server speaks HTTP/1.1
(svn_ra_serf__copy_into_spillbuf): copy a bucket's content into a
spillbuf, and return it
(svn_ra_serf__create_sb_bucket): create a bucket that reads out of a
spillbuf.
* subversion/libsvn_ra_serf/serf.c:
(svn_ra_serf__open): initialize the http10 flag (to FALSE for now)
* subversion/libsvn_ra_serf/update.c:
(open_connection_if_needed): initialize the http10 flag (to FALSE
for now)
* subversion/libsvn_ra_serf/sb_bucket.c: new file with functions to
copy a body into a spillbuf and to wrap a bucket around that.
* subversion/libsvn_ra_serf/util.c:
(svn_ra_serf__setup_serf_req): if we have a recent enough serf, then
examine the http10 flag in the connection, and possibly rejigger
the body bucket into a spillbuf. and tell the request bucket about
the length of the body so it can set the Content-Length header
Added:
subversion/trunk/subversion/libsvn_ra_serf/sb_bucket.c
Modified:
subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
subversion/trunk/subversion/libsvn_ra_serf/serf.c
subversion/trunk/subversion/libsvn_ra_serf/update.c
subversion/trunk/subversion/libsvn_ra_serf/util.c
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=1302682&r1=1302681&r2=1302682&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Mon Mar 19 22:04:36 2012
@@ -39,6 +39,7 @@
#include "svn_dirent_uri.h"
#include "private/svn_dav_protocol.h"
+#include "private/svn_subr_private.h"
#include "blncache.h"
@@ -70,6 +71,10 @@ typedef struct svn_ra_serf__connection_t
/* Our connection to a server. */
serf_connection_t *conn;
+ /* The server is not Apache/mod_dav_svn (directly) and only supports
+ HTTP/1.0. Thus, we cannot send chunked requests. */
+ svn_boolean_t http10;
+
/* Bucket allocator for this connection. */
serf_bucket_alloc_t *bkt_alloc;
@@ -1462,6 +1467,18 @@ svn_ra_serf__register_editor_shim_callba
svn_delta_shim_callbacks_t *callbacks);
+svn_error_t *
+svn_ra_serf__copy_into_spillbuf(svn_spillbuf_t **spillbuf,
+ serf_bucket_t *bkt,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+serf_bucket_t *
+svn_ra_serf__create_sb_bucket(svn_spillbuf_t *spillbuf,
+ serf_bucket_alloc_t *allocator,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Added: subversion/trunk/subversion/libsvn_ra_serf/sb_bucket.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/sb_bucket.c?rev=1302682&view=auto
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/sb_bucket.c (added)
+++ subversion/trunk/subversion/libsvn_ra_serf/sb_bucket.c Mon Mar 19 22:04:36 2012
@@ -0,0 +1,183 @@
+/*
+ * sb_bucket.c : a serf bucket that wraps a spillbuf
+ *
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ */
+
+#include <serf.h>
+#include <serf_bucket_util.h>
+
+#include "private/svn_subr_private.h"
+
+#include "ra_serf.h"
+
+#define SB_BLOCKSIZE 1024
+#define SB_MAXSIZE 32768
+
+
+struct sbb_baton
+{
+ svn_spillbuf_t *spillbuf;
+
+ const char *holding;
+ apr_size_t hold_len;
+
+ apr_pool_t *scratch_pool;
+};
+
+
+svn_error_t *
+svn_ra_serf__copy_into_spillbuf(svn_spillbuf_t **spillbuf,
+ serf_bucket_t *bkt,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ *spillbuf = svn_spillbuf__create(SB_BLOCKSIZE, SB_MAXSIZE, result_pool);
+
+ /* Copy all data from the bucket into the spillbuf. */
+ while (TRUE)
+ {
+ apr_status_t status;
+ const char *data;
+ apr_size_t len;
+
+ status = serf_bucket_read(bkt, SERF_READ_ALL_AVAIL, &data, &len);
+
+ /* ### we should throw an error, if the bucket does. */
+ SVN_ERR_ASSERT(status == APR_SUCCESS || status == APR_EOF);
+
+ SVN_ERR(svn_spillbuf__write(*spillbuf, data, len, scratch_pool));
+
+ if (status == APR_EOF)
+ break;
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
+static apr_status_t
+sb_bucket_read(serf_bucket_t *bucket, apr_size_t requested,
+ const char **data, apr_size_t *len)
+{
+ struct sbb_baton *sbb = bucket->data;
+ svn_error_t *err;
+
+ if (sbb->holding)
+ {
+ *data = sbb->holding;
+
+ if (requested < sbb->hold_len)
+ {
+ *len = requested;
+ sbb->holding += requested;
+ sbb->hold_len -= requested;
+ return APR_SUCCESS;
+ }
+
+ /* Return whatever we're holding, and then forget (consume) it. */
+ *len = sbb->hold_len;
+ sbb->holding = NULL;
+ return APR_SUCCESS;
+ }
+
+ err = svn_spillbuf__read(data, len, sbb->spillbuf, sbb->scratch_pool);
+ svn_pool_clear(sbb->scratch_pool);
+
+ /* ### do something with this */
+ svn_error_clear(err);
+
+ /* The spillbuf may have returned more than requested. Stash any extra
+ into our holding area. */
+ if (requested < *len)
+ {
+ sbb->holding = *data + requested;
+ sbb->hold_len = *len - requested;
+ *len = requested;
+ }
+
+ return *data == NULL ? APR_EOF : APR_SUCCESS;
+}
+
+
+static apr_status_t
+sb_bucket_readline(serf_bucket_t *bucket, int acceptable,
+ int *found,
+ const char **data, apr_size_t *len)
+{
+ /* ### for now, we know callers won't use this function. */
+ (void)svn_error__malfunction(TRUE, __FILE__, __LINE__, "Not implemented.");
+ return APR_ENOTIMPL;
+}
+
+
+static apr_status_t
+sb_bucket_peek(serf_bucket_t *bucket,
+ const char **data, apr_size_t *len)
+{
+ struct sbb_baton *sbb = bucket->data;
+ svn_error_t *err;
+
+ /* If we're not holding any data, then fill it. */
+ if (sbb->holding == NULL)
+ {
+ err = svn_spillbuf__read(&sbb->holding, &sbb->hold_len, sbb->spillbuf,
+ sbb->scratch_pool);
+ svn_pool_clear(sbb->scratch_pool);
+
+ /* ### do something with this */
+ svn_error_clear(err);
+ }
+
+ /* Return the data we are (now) holding. */
+ *data = sbb->holding;
+ *len = sbb->hold_len;
+
+ return *data == NULL ? APR_EOF : APR_SUCCESS;
+}
+
+
+static const serf_bucket_type_t sb_bucket_vtable = {
+ "SPILLBUF",
+ sb_bucket_read,
+ sb_bucket_readline,
+ serf_default_read_iovec,
+ serf_default_read_for_sendfile,
+ serf_default_read_bucket,
+ sb_bucket_peek,
+ serf_default_destroy_and_data,
+};
+
+
+serf_bucket_t *
+svn_ra_serf__create_sb_bucket(svn_spillbuf_t *spillbuf,
+ serf_bucket_alloc_t *allocator,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ struct sbb_baton *sbb;
+
+ sbb = serf_bucket_mem_alloc(allocator, sizeof(*sbb));
+ sbb->spillbuf = spillbuf;
+ sbb->holding = NULL;
+ sbb->scratch_pool = svn_pool_create(result_pool);
+
+ return serf_bucket_create(&sb_bucket_vtable, allocator, sbb);
+}
Modified: subversion/trunk/subversion/libsvn_ra_serf/serf.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/serf.c?rev=1302682&r1=1302681&r2=1302682&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/serf.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/serf.c Mon Mar 19 22:04:36 2012
@@ -397,16 +397,16 @@ svn_ra_serf__open(svn_ra_session_t *sess
serf_sess->conns[0] = apr_pcalloc(serf_sess->pool,
sizeof(*serf_sess->conns[0]));
+ serf_sess->conns[0]->http10 = TRUE; /* until we confirm HTTP/1.1 */
+ serf_sess->conns[0]->http10 = FALSE; /* ### don't change behavior yet */
serf_sess->conns[0]->bkt_alloc =
serf_bucket_allocator_create(serf_sess->pool, NULL, NULL);
serf_sess->conns[0]->session = serf_sess;
serf_sess->conns[0]->last_status_code = -1;
serf_sess->conns[0]->using_ssl = serf_sess->using_ssl;
- serf_sess->conns[0]->server_cert_failures = 0;
serf_sess->conns[0]->using_compression = serf_sess->using_compression;
serf_sess->conns[0]->hostname = url.hostname;
- serf_sess->conns[0]->useragent = NULL;
/* create the user agent string */
if (callbacks->get_client_string)
@@ -1196,6 +1196,7 @@ svn_ra_serf__init(const svn_version_t *l
|| serf_minor < SERF_MINOR_VERSION)
{
return svn_error_createf(
+ /* ### should return a unique error */
SVN_ERR_VERSION_MISMATCH, NULL,
_("ra_serf was compiled for serf %d.%d.%d but loaded "
"an incompatible %d.%d.%d library"),
Modified: subversion/trunk/subversion/libsvn_ra_serf/update.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/update.c?rev=1302682&r1=1302681&r2=1302682&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/update.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/update.c Mon Mar 19 22:04:36 2012
@@ -2226,16 +2226,17 @@ open_connection_if_needed(svn_ra_serf__s
int cur = sess->num_conns;
apr_status_t status;
- sess->conns[cur] = apr_palloc(sess->pool, sizeof(*sess->conns[cur]));
+ sess->conns[cur] = apr_pcalloc(sess->pool, sizeof(*sess->conns[cur]));
+ sess->conns[cur]->http10 = TRUE; /* until we confirm HTTP/1.1 */
+ sess->conns[cur]->http10 = FALSE; /* ### don't change behavior yet */
sess->conns[cur]->bkt_alloc = serf_bucket_allocator_create(sess->pool,
NULL, NULL);
sess->conns[cur]->hostname = sess->conns[0]->hostname;
sess->conns[cur]->using_ssl = sess->conns[0]->using_ssl;
sess->conns[cur]->using_compression = sess->conns[0]->using_compression;
- sess->conns[cur]->useragent = sess->conns[0]->useragent;
sess->conns[cur]->last_status_code = -1;
- sess->conns[cur]->ssl_context = NULL;
sess->conns[cur]->session = sess;
+ sess->conns[cur]->useragent = sess->conns[0]->useragent;
status = serf_connection_create2(&sess->conns[cur]->conn,
sess->context,
sess->session_url,
Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1302682&r1=1302681&r2=1302682&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/util.c Mon Mar 19 22:04:36 2012
@@ -638,13 +638,49 @@ svn_ra_serf__setup_serf_req(serf_request
const char *method, const char *url,
serf_bucket_t *body_bkt, const char *content_type)
{
+ serf_bucket_alloc_t *allocator = serf_request_get_alloc(request);
serf_bucket_t *hdrs_bkt;
+ svn_spillbuf_t *buf;
+
+ /* ### this should be passed */
+ apr_pool_t *scratch_pool = conn->session->pool;
+
+#if SERF_VERSION_AT_LEAST(1, 1, 0)
+ if (conn->http10 && body_bkt != NULL)
+ {
+ /* Ugh. Use HTTP/1.0 to talk to the server because we don't know if
+ it speaks HTTP/1.1 (and thus, chunked requests), or because the
+ server actually responded as only supporting HTTP/1.0.
+
+ We'll take the existing body_bkt, spool it into a spillbuf, and
+ then wrap a bucket around that spillbuf. The spillbuf will give
+ us the Content-Length value. */
+ SVN_ERR(svn_ra_serf__copy_into_spillbuf(&buf, body_bkt,
+ conn->session->pool,
+ scratch_pool));
+ body_bkt = svn_ra_serf__create_sb_bucket(buf, allocator,
+ conn->session->pool,
+ scratch_pool);
+ }
+#endif
/* Create a request bucket. Note that this sucker is kind enough to
add a "Host" header for us. */
*req_bkt =
serf_request_bucket_request_create(request, method, url, body_bkt,
- serf_request_get_alloc(request));
+ allocator);
+
+ /* Set the Content-Length value. This will also trigger an HTTP/1.0
+ request (rather than the default chunked request). */
+#if SERF_VERSION_AT_LEAST(1, 1, 0)
+ if (conn->http10)
+ {
+ if (body_bkt == NULL)
+ serf_bucket_request_set_CL(*req_bkt, 0);
+ else
+ serf_bucket_request_set_CL(*req_bkt, svn_spillbuf__get_size(buf));
+ }
+#endif
hdrs_bkt = serf_bucket_request_get_headers(*req_bkt);
serf_bucket_headers_setn(hdrs_bkt, "User-Agent", conn->useragent);
Re: svn commit: r1302682 - in
/subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h sb_bucket.c serf.c
update.c util.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Thu, Mar 22, 2012 at 13:19:41 -0400:
> On Thu, Mar 22, 2012 at 04:33, Julian Foad <ju...@btopenworld.com> wrote:
> > Greg Stein wrote:
> >> Daniel Shahaf wrote:
> >>> gstein@apache.org wrote:
> >>>> +svn_ra_serf__copy_into_spillbuf(svn_spillbuf_t **spillbuf,
> >>>> + serf_bucket_t *bkt,
> >>>> + apr_pool_t *result_pool,
> >>>> + apr_pool_t *scratch_pool)
> >>>> +{
> >>>> + status = serf_bucket_read(bkt, SERF_READ_ALL_AVAIL, &data, &len);
> >>>> +
> >>>> + /* ### we should throw an error, if the bucket does. */
> >>>> + SVN_ERR_ASSERT(status == APR_SUCCESS || status == APR_EOF);
> >>>
> >>> Can we please avoid these in new code?
> >>
> >> Why?
> >
> > Hi Greg. I'm puzzled by your response. You wrote "### we should throw an error" -- and now you're asking Daniel why?
>
> Daniel removed one of these ASSERT uses a day or two ago. My
> assumption was that he was referring to that, rather than the ###.
>
Yes, by "these" I referred to to the use of assert(), abort(), and
svn_error__malfunction().
Re: svn commit: r1302682 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h sb_bucket.c serf.c update.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Mar 22, 2012 at 04:33, Julian Foad <ju...@btopenworld.com> wrote:
> Greg Stein wrote:
>> Daniel Shahaf wrote:
>>> gstein@apache.org wrote:
>>>> +svn_ra_serf__copy_into_spillbuf(svn_spillbuf_t **spillbuf,
>>>> + serf_bucket_t *bkt,
>>>> + apr_pool_t *result_pool,
>>>> + apr_pool_t *scratch_pool)
>>>> +{
>>>> + status = serf_bucket_read(bkt, SERF_READ_ALL_AVAIL, &data, &len);
>>>> +
>>>> + /* ### we should throw an error, if the bucket does. */
>>>> + SVN_ERR_ASSERT(status == APR_SUCCESS || status == APR_EOF);
>>>
>>> Can we please avoid these in new code?
>>
>> Why?
>
> Hi Greg. I'm puzzled by your response. You wrote "### we should throw an error" -- and now you're asking Daniel why?
Daniel removed one of these ASSERT uses a day or two ago. My
assumption was that he was referring to that, rather than the ###.
>> I see no problem here: it will just bail out with an error.
>
> Not true in general; only if the application writer explicitly sets the malfunction handler to do so.
I'm assuming that all applications *do* set it to return. We use
SVN_ERR_ASSERT() liberally upon that assumption.
> Anyway, the point is not only about how the application exits, but about writing code that says what we mean.
I have no idea what you mean here.
Cheers,
-g
Re: svn commit: r1302682 - in /subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h sb_bucket.c serf.c update.c util.c
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Stein wrote:
> Daniel Shahaf wrote:
>> gstein@apache.org wrote:
>>> +svn_ra_serf__copy_into_spillbuf(svn_spillbuf_t **spillbuf,
>>> + serf_bucket_t *bkt,
>>> + apr_pool_t *result_pool,
>>> + apr_pool_t *scratch_pool)
>>> +{
>>> + status = serf_bucket_read(bkt, SERF_READ_ALL_AVAIL, &data, &len);
>>> +
>>> + /* ### we should throw an error, if the bucket does. */
>>> + SVN_ERR_ASSERT(status == APR_SUCCESS || status == APR_EOF);
>>
>> Can we please avoid these in new code?
>
> Why?
Hi Greg. I'm puzzled by your response. You wrote "### we should throw an error" -- and now you're asking Daniel why?
> I see no problem here: it will just bail out with an error.
Not true in general; only if the application writer explicitly sets the malfunction handler to do so.
Anyway, the point is not only about how the application exits, but about writing code that says what we mean.
- Julian
Re: svn commit: r1302682 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h sb_bucket.c serf.c update.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Mar 21, 2012 at 17:16, Daniel Shahaf <da...@elego.de> wrote:
> gstein@apache.org wrote on Mon, Mar 19, 2012 at 22:04:37 -0000:
>> +svn_ra_serf__copy_into_spillbuf(svn_spillbuf_t **spillbuf,
>> + serf_bucket_t *bkt,
>> + apr_pool_t *result_pool,
>> + apr_pool_t *scratch_pool)
>> +{
>> + status = serf_bucket_read(bkt, SERF_READ_ALL_AVAIL, &data, &len);
>> +
>> + /* ### we should throw an error, if the bucket does. */
>> + SVN_ERR_ASSERT(status == APR_SUCCESS || status == APR_EOF);
>
> Can we please avoid these in new code?
Why? I see no problem here: it will just bail out with an error. It is
the NO_RETURN() form that causes an abort() because it has nowhere
else to go.
Cheers,
-g
Re: svn commit: r1302682 - in
/subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h sb_bucket.c serf.c
update.c util.c
Posted by Daniel Shahaf <da...@elego.de>.
gstein@apache.org wrote on Mon, Mar 19, 2012 at 22:04:37 -0000:
> +svn_ra_serf__copy_into_spillbuf(svn_spillbuf_t **spillbuf,
> + serf_bucket_t *bkt,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + status = serf_bucket_read(bkt, SERF_READ_ALL_AVAIL, &data, &len);
> +
> + /* ### we should throw an error, if the bucket does. */
> + SVN_ERR_ASSERT(status == APR_SUCCESS || status == APR_EOF);
Can we please avoid these in new code?
Re: svn commit: r1302682 - in
/subversion/trunk/subversion/libsvn_ra_serf: ra_serf.h sb_bucket.c serf.c
update.c util.c
Posted by Daniel Shahaf <da...@elego.de>.
gstein@apache.org wrote on Mon, Mar 19, 2012 at 22:04:37 -0000:
> +svn_ra_serf__copy_into_spillbuf(svn_spillbuf_t **spillbuf,
> + serf_bucket_t *bkt,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + status = serf_bucket_read(bkt, SERF_READ_ALL_AVAIL, &data, &len);
> +
> + /* ### we should throw an error, if the bucket does. */
> + SVN_ERR_ASSERT(status == APR_SUCCESS || status == APR_EOF);
Can we please avoid these in new code?