> -----Original Message-----
> From: Greg Stein [mailto:gst...@gmail.com]
> Sent: donderdag 12 november 2015 01:44
> To: Bert Huijben <rhuij...@apache.org>
> Cc: dev@serf.apache.org
> Subject: Re: svn commit: r1713936 - in /serf/trunk: buckets/allocator.c
> buckets/log_wrapper_buckets.c outgoing.c test/mock_buckets.c
>
> On Wed, Nov 11, 2015 at 3:29 PM, <rhuij...@apache.org> wrote:
> >...
>
> > +++ serf/trunk/buckets/allocator.c Wed Nov 11 21:29:45 2015
> > @@ -416,9 +416,11 @@ apr_status_t serf_debug__record_read(
> > read_status_t *rs = find_read_status(track, bucket, 1);
> >
> > /* Validate that the previous status value allowed for another read.
> > */
> > - if (APR_STATUS_IS_EAGAIN(rs->last) /* ### or APR_EOF? */) {
> > + if (SERF_BUCKET_READ_ERROR(rs->last)) {
> >
>
> This is wrong. If a bucket returns APR_EAGAIN, you are *not* supposed to
> read the bucket again until the context loop has been invoked.
>
> I agree that you should stop reading if a READ_ERROR occurs. That is a good
> addition. But that excludes EAGAIN, incorrectly.
This assumed somebody calls serf_debug__entered_loop()...
Which we -as far as I can tell- never did. No call in trunk nor in
branches/[01].[0-9].x.
I have a patch that adds this call on the connection and the allocators of
(partially) written request.
But adding that call caused new test failures, especially around ssl buckets,
where substreams are commonly not read further after APR_SUCCESS... as we have
to wait for the server to come with more data first, before we can write
something else.
(Trying to work out a solution)
> > @@ -492,6 +497,27 @@ void serf_debug__bucket_destroy(const se
> > if (SERF_BUCKET_IS_BARRIER(bucket))
> > return;
> >
> > + if (SERF_BUCKET_IS_AGGREGATE(bucket)) {
> > + apr_status_t status;
> > + const char *data;
> > + apr_size_t len;
> > +
> > + serf_bucket_aggregate_hold_open(bucket, NULL, NULL);
> > +
> > + status = serf_bucket_read(bucket, SERF_READ_ALL_AVAIL,
> > + &data, &len);
> > +
> > + if (APR_STATUS_IS_EOF(status) && !len)
> > + return;
> > + }
> > +
> > + bkt = serf_bucket_read_bucket((serf_bucket_t*)bucket,
> > + &serf_bucket_type_ssl_encrypt);
> > +
> > + if (bkt != NULL) {
> > + serf_bucket_destroy(bkt);
> > + return;
> > + }
> >
>
> What is all that?? How about some comments, please.
Aggregate buckets can have a hold open callback, causing them to return
something else than EOF (E.g. EAGAIN) until <some condition>.
Without this check the check just fails in dozens of testcases.
And there are also buckets that don't return EOF when exactly the amount of
data stored in them is read. Only the next read will show the difference
between those two cases.
(And aggregates containing ssl encrypt buckets fail quite a few other
conditions)
Bert
