[PATCH, WIP] Add serf_bucket_get_remaining()

[Ivan Zhakov]

Hi,

It already discussed that would be useful to add optional vtable
method to buckets to return length of available in bucket. It seems to
be easy to implement without breaking compatibility. See attached
patch.

Comments? Objections?

[[[
Add new bucket vtable member get_remaining() to obtain remaining data
length in bucket.

* buckets/aggregate_buckets.c
(serf_aggregate_get_remaining): New.
(serf_bucket_type_aggregate): Add serf_bucket_type_aggregate().
* buckets/simple_buckets.c
(serf_simple_get_remaining): New.
(serf_bucket_type_simple): Add serf_simple_get_remaining().
* serf.h
(SERF_LENGTH_UNKNOWN): New.
(serf_bucket_type_t): Add get_remaining vtable member.
(serf_bucket_get_remaining): New.
* test/test_buckets.c
(test_aggregate_buckets): Test serf_bucket_get_remaining() for aggregate
bucket.
]]]

--
Ivan Zhakov

[Greg Stein]

On Mon, Jul 1, 2013 at 10:40 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> Hi,
>
> It already discussed that would be useful to add optional vtable
> method to buckets to return length of available in bucket. It seems to
> be easy to implement without breaking compatibility. See attached
> patch.
>
> Comments? Objections?

Let's say that we add this in serf 1.4. And say an app using serf's
1.0 API has some custom bucket types. serf would have no idea that the
(new) vtable entry is not present. (note that I'm assuming serf will
start to use get_remaining internally; thus, we can't say "no crashes
cuz old apps won't use the new vtable entry").

We need a way to tell serf the bucket API version in use.

Here is a total hack on how to do that:

Note that read_bucket() is not called by anybody today, afaik, so we
will slow it down a tiny bit and use it for versioning:

serf_bucket_t * serf_buckets_are_v2(
serf_bucket_t *bucket,
const serf_bucket_type_t *type)
{
return bucket->read_bucket_v2(bucket, type);
}

apr_uint64_t serf_bucket_get_remaining(serf_bucket_t *bucket)
{
if (bucket->read_bucket == serf_buckets_are_v2
&& bucket->get_remaining != NULL)
return bucket->get_remaining(bucket);
return SERF_LENGTH_UNKNOWN;
}

So: we'd add two new vtable entry points: get_remaining and
read_bucket_v2. Implementors would set their read_bucket entrypoint to
serf_buckets_are_v2 to indicate they've switched over to the new API.

If we add more vtable entries, then we just add serf_buckets_are_v3 / _v4 / etc.

Total hack. I know. But it should work in a compatible fashion.

Thoughts?

Cheers,
-g

[Ivan Zhakov]

Ok, now I got the problem :)

read_bucket() actively used in Subversion code.

So I have alternative approach. First of all add VERSION member to
serf_bucket. Then add serf_bucket_version() function that return 1 for
all buckets except serf internal buckets for which we define VERSION
member in vtable. Something like this:

int serf_bucket_version(serf_bucket_t *b)
{
if (SERF_BUCKET_IS_SIMPLE(b) || SERF_BUCKET_IS_AGGREGATE(b))
{
return b->type->version;
}
else
{
return 1;
}
}


And then:
apr_uint64_t serf_bucket_get_remaining(serf_bucket_t *bucket)
{
if (serf_bucket_version >= 2)
return bucket->type->get_remaining();
else
return SERF_LENGTH_UNKNOWN;
}

Thoughts?


--
Ivan Zhakov

[Greg Stein]

On Wed, Jul 3, 2013 at 5:03 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>...

> read_bucket() actively used in Subversion code.

Eh? No it isn't. Nobody *calls* that function. That's why I suggest we
can make it a bit slower in order to providing some versioning within
the 1.x API space.

> So I have alternative approach. First of all add VERSION member to
> serf_bucket. Then add serf_bucket_version() function that return 1 for
> all buckets except serf internal buckets for which we define VERSION
> member in vtable. Something like this:
>
> int serf_bucket_version(serf_bucket_t *b)
> {
> if (SERF_BUCKET_IS_SIMPLE(b) || SERF_BUCKET_IS_AGGREGATE(b))

Euh... this doesn't work. How does a third party bucket say they
support the new API?

>...

The (hack) proposal that I suggested allows us to avoid these kinds of
specific tests and it also allows for third-party buckets to continue
to function, and for those buckets to upgrade to the v2 specification.

I still think my idea is a hack.Would be happy to replace. But I think
we really need something that allows 3rd-party buckets to continue
work (API compat), and for 3rd-party buckets to use the new API at
their choice. WIth the latter, it means that we can incrementally
improve our own buckets to use the new API, since each bucket can
choose (independently) whether to use v1 or v2 of the bucket API.

Cheers,
-g

[Ivan Zhakov]

On Wed, Jul 3, 2013 at 1:17 PM, Greg Stein <gst...@gmail.com> wrote:
> On Wed, Jul 3, 2013 at 5:03 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>...
>> read_bucket() actively used in Subversion code.
>
> Eh? No it isn't. Nobody *calls* that function. That's why I suggest we
> can make it a bit slower in order to providing some versioning within
> the 1.x API space.
>
>> So I have alternative approach. First of all add VERSION member to
>> serf_bucket. Then add serf_bucket_version() function that return 1 for
>> all buckets except serf internal buckets for which we define VERSION
>> member in vtable. Something like this:
>>
>> int serf_bucket_version(serf_bucket_t *b)
>> {
>> if (SERF_BUCKET_IS_SIMPLE(b) || SERF_BUCKET_IS_AGGREGATE(b))
>
> Euh... this doesn't work. How does a third party bucket say they
> support the new API?
>

They cannot in my proposal. But in reality there is know third-party
buckets. And we could revv our API to add explicit version in serf
2.0.

> The (hack) proposal that I suggested allows us to avoid these kinds of
> specific tests and it also allows for third-party buckets to continue
> to function, and for those buckets to upgrade to the v2 specification.
>
> I still think my idea is a hack.Would be happy to replace. But I think
> we really need something that allows 3rd-party buckets to continue
> work (API compat), and for 3rd-party buckets to use the new API at
> their choice. WIth the latter, it means that we can incrementally
> improve our own buckets to use the new API, since each bucket can
> choose (independently) whether to use v1 or v2 of the bucket API.
>

My proposal is also hack but a little bit easier IMHO. Just to solve
C-L problem.


--
Ivan Zhakov

[Greg Stein]

On Wed, Jul 3, 2013 at 5:28 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Wed, Jul 3, 2013 at 1:17 PM, Greg Stein <gst...@gmail.com> wrote:

>>...

>> Euh... this doesn't work. How does a third party bucket say they
>> support the new API?
>>
> They cannot in my proposal. But in reality there is know third-party
> buckets.

libsvn_ra_serf/sb_bucket.c

And I'd like to add more.

> And we could revv our API to add explicit version in serf 2.0.

Definitely!!

>...

Cheers,
-g

[Ivan Zhakov]

On Wed, Jul 3, 2013 at 1:40 PM, Greg Stein <gst...@gmail.com> wrote:
> On Wed, Jul 3, 2013 at 5:28 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On Wed, Jul 3, 2013 at 1:17 PM, Greg Stein <gst...@gmail.com> wrote:
>>>...
>>> Euh... this doesn't work. How does a third party bucket say they
>>> support the new API?
>>>
>> They cannot in my proposal. But in reality there is know third-party
>> buckets.
>
> libsvn_ra_serf/sb_bucket.c
>

Oops, I forgot about it.

> And I'd like to add more.
>

>> And we could revv our API to add explicit version in serf 2.0.
>
> Definitely!!
>

What is the best way to record idea to add VERSION member to bucket
vtable? Or I can just add to trunk since it's already 2.0?

--
Ivan Zhakov

[Ivan Zhakov]

On Wed, Jul 3, 2013 at 1:17 PM, Greg Stein <gst...@gmail.com> wrote:

> On Wed, Jul 3, 2013 at 5:03 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>...
>> read_bucket() actively used in Subversion code.
>
> Eh? No it isn't. Nobody *calls* that function. That's why I suggest we
> can make it a bit slower in order to providing some versioning within
> the 1.x API space.
>

I actually mixed up read() and read_bucket(). read_bucket() is not
used in serf and Subversion code. I'll implement serf_bucket_version()
using your proposal then.


--
Ivan Zhakov

[Ivan Zhakov]

Hi Greg,

I've implemented bucket types vtable versioning using special
read_bucket() function. See attached patch.

I decided add explicit version field to buckets to be able write something like:
[[[
if (serf_bucket_version() >= 2) {
serf_bucket_get_remaining();
}
]]]

[[[
* serf.h
(serf_bucket_type_t): Add VERSION and READ_BUCKET_V2 members.
(serf_bucket_has_version): Declare.
(serf_bucket_version): New.
* buckets/buckets.c
(serf_bucket_type_simple): Declare as versioned bucket type.
* buckets/simple_buckets.c
(serf_bucket_has_version): Implement.
]]]

--
Ivan Zhakov

[Greg Stein]

On Wed, Jul 3, 2013 at 9:03 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>...

> I've implemented bucket types vtable versioning using special
> read_bucket() function. See attached patch.

Cool!

> I decided add explicit version field to buckets to be able write something like:

It seems that we're only working on a mid-term hack. I don't forsee
any other bucket changes before we move to 2.x. *Maybe* we'll add a
version then, but a version today seem a bit much.

My concern is that exposing a "version" in a bucket type will make it
*too easy* to consider changing the bucket vtable. That structure is
one of the core pieces of serf, and it should remain highly stable.

At the moment, we're simply looking to add get_remaining(). Nothing more.

And note: nothing *has* been identified for the bucket in the past
seven(?) years, beyond this one vtable entry. I think we're safe
making an exception for this one entry, and move along. If the sky
falls down next year, and we find another reason to add another vtable
slot, then we know how to do that. But it seems dangerous to add a
version field, leading people to think "hey! no big deal! add more
callbacks!"

We've spun off the branch for 1.3.x. I think we can add a
get_remaining() for 1.4.x, but please narrow the patch.

Thanks!
-g

[Ivan Zhakov]

Makes sense for me.

I've added serf_bucket_get_remaining() function and implemented it for
AGGREGATE, BARRIER, FILE, MMAP and SIMPLE buckets in r2008.

The next question how to plug it to REQUEST bucket:
1. Let application to query request body bucket for length and then
set it using serf_bucket_request_set_CL()

2. Teach REQUEST bucket to query body bucket for length and use it
automatically.

Thoughts?

--
Ivan Zhakov