After thinking it through a few more times applied the suggested
change/revert/… in r1713906.
Thanks,
Bert
From: Greg Stein [mailto:gst...@gmail.com]
Sent: woensdag 11 november 2015 18:23
To: Bert Huijben <b...@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 <b...@qqmail.nl
<mailto:b...@qqmail.nl> > wrote:
>...
> -----Original Message-----
> From: Greg Stein [mailto:gst...@gmail.com <mailto:gst...@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
