serf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bert Huijben" <b...@qqmail.nl>
Subject RE: svn commit: r1713936 - in /serf/trunk: buckets/allocator.c buckets/log_wrapper_buckets.c outgoing.c test/mock_buckets.c
Date Thu, 12 Nov 2015 12:37:56 GMT


> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com]
> Sent: donderdag 12 november 2015 01:44
> To: Bert Huijben <rhuijben@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, <rhuijben@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


Mime
View raw message