Hi Greg, Thanks for your reply.
> -----Original Message----- > From: Greg Stein [mailto:gst...@gmail.com] > Sent: woensdag 11 november 2015 17:05 > To: dev@serf.apache.org > Subject: limited readline (was: svn commit: r1713489 - > serf_bucket_readline()) > > On Tue, Nov 10, 2015 at 4:44 PM, Bert Huijben <b...@qqmail.nl> wrote: > >... > > > There is one more problem with all this wrapping: All layers need to > > somehow support all read methods. > > > > Wrapping works for all methods except: readline. > > > > If all methods are passed to the wrapped bucket, then readline works fine. > > > > This method is only implemented on a few bucket types, and doesn't allow > > passing an upper limit to whatever is read... so things like limit and > > unframing buckets can never implement this without implementing some > > caching... > > > > And once they introduce caching all read methods need to look at the > cache. > > > > Sure. That's what 'databuf' is all about. To implement that cache in a > general way. > > Does it play nice with "limit" type buckets? Well, no... it overreads. > Those buckets will need to be *much* smarter. > > 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. 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. (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) > Because each read() will be preceded by a peek(), I think adding a > peek_iovecs() can improve the performance. It can possibly provide more > data to find that CR/LF. It's not required, but it might be a nice addition. [[I'm surprised how many bugs I've found in peek implementations over the last few months... Producing too much data, or missing some data] > 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 :( > 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. I explicitly documented that most callers should continue calling the normal readline. The pain is now on implementers of limiting buckets and those that want to implement other features that are bucket-v2 only. (The non http2 limiting buckets are upgraded now) Bert
