serf-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@gmail.com>
Subject Re: limited readline (was: svn commit: r1713489 - serf_bucket_readline())
Date Wed, 11 Nov 2015 17:22:35 GMT
On Wed, Nov 11, 2015 at 11:00 AM, Bert Huijben <bert@qqmail.nl> wrote:
>...

> > -----Original Message-----
> > From: Greg Stein [mailto:gstein@gmail.com]
> > Sent: woensdag 11 november 2015 17:05
> > To: 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