serf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bert Huijben" <b...@qqmail.nl>
Subject RE: limited readline (was: svn commit: r1713489 - serf_bucket_readline())
Date Wed, 11 Nov 2015 17:37:30 GMT
[Somehow we switched to HTML mails… And I’m not even using my tablet now].

 

I think switching to a helper function would work for all current use cases. (Still trying
to make up my mind which solution I would prefer)

 

 

The only case where it would really work against us if you start mixing buckets that implement
something like a circular buffer with limiting. In those scenarios the additional entry point
would make things easier.

 

If the bucket could restructure data to return more at once based on the EOL information,
passing through the information really helps.

 

 

But when the common datasources are databuffers (sockets, files), or encryption/compression
buckets just using the helper function with a good peek works fine. And circular buffers are
really not a design that we would like to promote with serf’s zero copying system.

 

 

BTW: r1711950 fixed the worst case of a peek implementation failure I can remember. There
were a few more, but this one really produced bad data.

 

                Bert

 

 

From: Greg Stein [mailto:gstein@gmail.com] 
Sent: woensdag 11 november 2015 18:23
To: Bert Huijben <bert@qqmail.nl>
Cc: dev@serf.apache.org
Subject: Re: limited readline (was: svn commit: r1713489 - serf_bucket_readline())

 

On Wed, Nov 11, 2015 at 11:00 AM, Bert Huijben <bert@qqmail.nl <mailto:bert@qqmail.nl>
> wrote:

>...

> -----Original Message-----
> From: Greg Stein [mailto:gstein@gmail.com <mailto:gstein@gmail.com> ]
> Sent: woensdag 11 november 2015 17:05
> To: dev@serf.apache.org <mailto:dev@serf.apache.org> 
> Subject: limited readline (was: svn commit: r1713489 -
> serf_bucket_readline())

>... 

> I believe the correct answer is that limit-type buckets need to peek first,
> look for CR and/or LF, and then read() that much to return the caller.
> Limited, as appropriate.

One problem here is that implementing peek is optional too.
See r1713834, where the current linebuffer code can get in an endless APR_EAGAIN return mode
waiting for data to peek.

 

Saw that, yes. And that is a problem with linebuf. It is *not* a problem for readline, since
the latter can peek-first, then default to read() of a single character.

 

linebuf is trying to deal with CRLF_SPLIT and *cannot* over-read. We cannot read one character
and say, "previous line is ready, ending with CR. new line starts with $char."

 

But readline is allowed to do a destructive read. So when peek returns 0, then it can read
1. (and maybe peek again, assuming the underlying bucket now has been "filled" with more data)

 

As the linebuf type is not really opaque and we can't really re-add something that we already
read, the best solution I could find was trying to fix the other buckets to implement peek
support where missing.

 

Yeah :-/

 

Seems we need to find a better answer for linebuf.

 

(Only remaining bucket is the BTWP bucket. I'm not sure where this is really used and if it
will have much future after HTTP/2. Didn't investigate)

 

Yup. Maybe deprecate the bucket, but try in the meantime, see if a non-zero length peek can
be implemented.

 

>... 

> I disagree with the recently-added readline2() because it's use is *so*
> minimal. It would be unfortunate to have two entry points with such similar
> signatures.
>
> We could add a utility function: serf_bucket_limited_readline() that is a
> cover over peek/readline.

See: the default peek as nothing available implementation problem :(

 

Solved :-)

 

> This problem is not that relevant for writing requests, as that reads raw
> > data anyway... but we are having more and more buckets that just set the
> > readline implementation to NULL.
> >
>
> Well, that was just them being lazy :-P ... I see you're fixing some of
> those buckets.

I think most of them are now fixed.

I 100% agree that adding a so slightly different entrypoint is not a nice solution.

 

How about we switch to the limited_readline() utility function (basically rename serf_default_readline2),
and then remove the bucket entrypoint?

 

Cheers,

-g

 


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message