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 18:09:45 GMT
After thinking it through a few more times applied the suggested change/revert/… in r1713906.

 

Thanks,

                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