RE: svn commit: r1713489 - serf_bucket_readline()
> -Original Message- > From: Bert Huijben [mailto:b...@qqmail.nl] > Sent: dinsdag 10 november 2015 23:37 > To: 'Ivan Zhakov' ; rhuij...@apache.org > Cc: dev@serf.apache.org > Subject: RE: svn commit: r1713489 - in /serf/trunk: buckets/event_buckets.c > outgoing.c serf_private.h > > > > > -Original Message- > > From: Ivan Zhakov [mailto:i...@visualsvn.com] > > Sent: dinsdag 10 november 2015 20:43 > > To: rhuij...@apache.org > > Cc: dev@serf.apache.org > > Subject: Re: svn commit: r1713489 - in /serf/trunk: > buckets/event_buckets.c > > outgoing.c serf_private.h > > > > On 9 November 2015 at 20:49, wrote: > > > Author: rhuijben > > > Date: Mon Nov 9 17:49:59 2015 > > > New Revision: 1713489 > > > > > > URL: http://svn.apache.org/viewvc?rev=1713489&view=rev > > > Log: > > > Replace the track bucket in the request writing with a tiny bit more > > > advanced event bucket that tracks both writing done and destroyed. The > > > timings of these callbacks will allow simplifying some logic introduced > > > in r1712776. > > > > > > For now declare the event bucket as a private type. > > > > > > * buckets/event_buckets.c > > > New file. > > > > > > * outgoing.c > > > (request_writing_done): New function. > > > (request_writing_finished): Tweak to implement > > serf_bucket_event_callback_t. > > > (write_to_connection): Add event bucket directly after the request > > bucket, > > > instead of an aggregate when the writing is done. > > > > > Hi Bert, > > > > What do you think about alternative design for event buckets: make > > event bucket wrap any other bucket (request bucket in our case)? I > > think it will be more flexible since we could add callback to be > > called before reading from wrapped bucket to track pending request. > > That might work, but would have different characteristics unless you do > more special things. 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. 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. 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. Perhaps we can improve things a bit by adding an improved readline to the buckets v2, but I'm not sure how yet. (Would passing a new max size argument be enough?) Bert
Re: svn commit: r1713489 - serf_bucket_readline()
On 11 November 2015 at 01:44, Bert Huijben wrote: >> -Original Message- >> From: Bert Huijben [mailto:b...@qqmail.nl] >> Sent: dinsdag 10 november 2015 23:37 >> To: 'Ivan Zhakov' ; rhuij...@apache.org >> Cc: dev@serf.apache.org >> Subject: RE: svn commit: r1713489 - in /serf/trunk: buckets/event_buckets.c >> outgoing.c serf_private.h >> > -Original Message- >> > From: Ivan Zhakov [mailto:i...@visualsvn.com] >> > Sent: dinsdag 10 november 2015 20:43 >> > To: rhuij...@apache.org >> > Cc: dev@serf.apache.org >> > Subject: Re: svn commit: r1713489 - in /serf/trunk: >> buckets/event_buckets.c >> > outgoing.c serf_private.h >> > >> > On 9 November 2015 at 20:49, wrote: >> > > Author: rhuijben >> > > Date: Mon Nov 9 17:49:59 2015 >> > > New Revision: 1713489 >> > > >> > > URL: http://svn.apache.org/viewvc?rev=1713489&view=rev >> > > Log: >> > > Replace the track bucket in the request writing with a tiny bit more >> > > advanced event bucket that tracks both writing done and destroyed. The >> > > timings of these callbacks will allow simplifying some logic introduced >> > > in r1712776. >> > > >> > > For now declare the event bucket as a private type. >> > > >> > > * buckets/event_buckets.c >> > > New file. >> > > >> > > * outgoing.c >> > > (request_writing_done): New function. >> > > (request_writing_finished): Tweak to implement >> > serf_bucket_event_callback_t. >> > > (write_to_connection): Add event bucket directly after the request >> > bucket, >> > > instead of an aggregate when the writing is done. >> > > >> > Hi Bert, >> > >> > What do you think about alternative design for event buckets: make >> > event bucket wrap any other bucket (request bucket in our case)? I >> > think it will be more flexible since we could add callback to be >> > called before reading from wrapped bucket to track pending request. >> >> That might work, but would have different characteristics unless you do >> more special things. > > 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. > How is event bucket situation different from barrier bucket for example? -- Ivan Zhakov
Re: svn commit: r1713489 - serf_bucket_readline()
On Wed, Nov 11, 2015 at 6:21 AM, Ivan Zhakov wrote: >... > How is event bucket situation different from barrier bucket for example? > Barrier buckets wrap a bucket and pass all methods through to it. They do *not* destroy the wrapped bucket when the barrier is destroyed. Event bucket is a zero-length content bucket with a couple callbacks. Cheers, -g
Re: svn commit: r1713489 - serf_bucket_readline()
On 11 November 2015 at 18:34, Greg Stein wrote: > On Wed, Nov 11, 2015 at 6:21 AM, Ivan Zhakov wrote: >>... >> >> How is event bucket situation different from barrier bucket for example? > > > Barrier buckets wrap a bucket and pass all methods through to it. They do > *not* destroy the wrapped bucket when the barrier is destroyed. > > Event bucket is a zero-length content bucket with a couple callbacks. > I meant how it would be different if event bucket will wrap existing bucket passing all methods through it. -- Ivan Zhakov
RE: svn commit: r1713489 - serf_bucket_readline()
> -Original Message- > From: Ivan Zhakov [mailto:i...@visualsvn.com] > Sent: woensdag 11 november 2015 13:22 > To: Bert Huijben > Cc: rhuij...@apache.org; dev@serf.apache.org > Subject: Re: svn commit: r1713489 - serf_bucket_readline() > > On 11 November 2015 at 01:44, Bert Huijben wrote: > >> -Original Message- > >> From: Bert Huijben [mailto:b...@qqmail.nl] > >> Sent: dinsdag 10 november 2015 23:37 > >> To: 'Ivan Zhakov' ; rhuij...@apache.org > >> Cc: dev@serf.apache.org > >> Subject: RE: svn commit: r1713489 - in /serf/trunk: > buckets/event_buckets.c > >> outgoing.c serf_private.h > >> > -Original Message- > >> > From: Ivan Zhakov [mailto:i...@visualsvn.com] > >> > Sent: dinsdag 10 november 2015 20:43 > >> > To: rhuij...@apache.org > >> > Cc: dev@serf.apache.org > >> > Subject: Re: svn commit: r1713489 - in /serf/trunk: > >> buckets/event_buckets.c > >> > outgoing.c serf_private.h > >> > > >> > On 9 November 2015 at 20:49, wrote: > >> > > Author: rhuijben > >> > > Date: Mon Nov 9 17:49:59 2015 > >> > > New Revision: 1713489 > >> > > > >> > > URL: http://svn.apache.org/viewvc?rev=1713489&view=rev > >> > > Log: > >> > > Replace the track bucket in the request writing with a tiny bit more > >> > > advanced event bucket that tracks both writing done and destroyed. > The > >> > > timings of these callbacks will allow simplifying some logic introduced > >> > > in r1712776. > >> > > > >> > > For now declare the event bucket as a private type. > >> > > > >> > > * buckets/event_buckets.c > >> > > New file. > >> > > > >> > > * outgoing.c > >> > > (request_writing_done): New function. > >> > > (request_writing_finished): Tweak to implement > >> > serf_bucket_event_callback_t. > >> > > (write_to_connection): Add event bucket directly after the request > >> > bucket, > >> > > instead of an aggregate when the writing is done. > >> > > > >> > Hi Bert, > >> > > >> > What do you think about alternative design for event buckets: make > >> > event bucket wrap any other bucket (request bucket in our case)? I > >> > think it will be more flexible since we could add callback to be > >> > called before reading from wrapped bucket to track pending request. > >> > >> That might work, but would have different characteristics unless you do > >> more special things. > > > > 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. > > > How is event bucket situation different from barrier bucket for example? The event bucket currently doesn't forward a single read method, while the barrier forwards all? Let me reverse the question: what makes them similar? I think there are two classes of buckets: one that wrap other bucket(s) in some way or another and others that don't. The current event bucket falls in that last category, just like the simple, the iovec, the memmap, the file and the socket buckets (and probably a few others). Bert
Re: svn commit: r1713489 - serf_bucket_readline()
On Wed, Nov 11, 2015 at 10:51 AM, Bert Huijben wrote: > > > > -Original Message- > > From: Ivan Zhakov [mailto:i...@visualsvn.com] > > Sent: woensdag 11 november 2015 13:22 > > To: Bert Huijben > > Cc: rhuij...@apache.org; dev@serf.apache.org > > Subject: Re: svn commit: r1713489 - serf_bucket_readline() > > > > On 11 November 2015 at 01:44, Bert Huijben wrote: > > >> -Original Message- > > >> From: Bert Huijben [mailto:b...@qqmail.nl] > > >> Sent: dinsdag 10 november 2015 23:37 > > >> To: 'Ivan Zhakov' ; rhuij...@apache.org > > >> Cc: dev@serf.apache.org > > >> Subject: RE: svn commit: r1713489 - in /serf/trunk: > > buckets/event_buckets.c > > >> outgoing.c serf_private.h > > >> > -Original Message- > > >> > From: Ivan Zhakov [mailto:i...@visualsvn.com] > > >> > Sent: dinsdag 10 november 2015 20:43 > > >> > To: rhuij...@apache.org > > >> > Cc: dev@serf.apache.org > > >> > Subject: Re: svn commit: r1713489 - in /serf/trunk: > > >> buckets/event_buckets.c > > >> > outgoing.c serf_private.h > > >> > > > >> > On 9 November 2015 at 20:49, wrote: > > >> > > Author: rhuijben > > >> > > Date: Mon Nov 9 17:49:59 2015 > > >> > > New Revision: 1713489 > > >> > > > > >> > > URL: http://svn.apache.org/viewvc?rev=1713489&view=rev > > >> > > Log: > > >> > > Replace the track bucket in the request writing with a tiny bit > more > > >> > > advanced event bucket that tracks both writing done and destroyed. > > The > > >> > > timings of these callbacks will allow simplifying some logic > introduced > > >> > > in r1712776. > > >> > > > > >> > > For now declare the event bucket as a private type. > > >> > > > > >> > > * buckets/event_buckets.c > > >> > > New file. > > >> > > > > >> > > * outgoing.c > > >> > > (request_writing_done): New function. > > >> > > (request_writing_finished): Tweak to implement > > >> > serf_bucket_event_callback_t. > > >> > > (write_to_connection): Add event bucket directly after the > request > > >> > bucket, > > >> > > instead of an aggregate when the writing is done. > > >> > > > > >> > Hi Bert, > > >> > > > >> > What do you think about alternative design for event buckets: make > > >> > event bucket wrap any other bucket (request bucket in our case)? I > > >> > think it will be more flexible since we could add callback to be > > >> > called before reading from wrapped bucket to track pending request. > > >> > > >> That might work, but would have different characteristics unless you > do > > >> more special things. > > > > > > 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. > > > > > How is event bucket situation different from barrier bucket for example? > > The event bucket currently doesn't forward a single read method, while the > barrier forwards all? > > > Let me reverse the question: what makes them similar? > > I think there are two classes of buckets: one that wrap other bucket(s) in > some way or another and others that don't. > > The current event bucket falls in that last category, just like the > simple, the iovec, the memmap, the file and the socket buckets (and > probably a few others). > Sure, but you could use one callback when the wrapped bucket hits EOF, and the other callback when the wrapper is destroyed. A wrapper can be more efficient, as a content bucket generally requires an aggregate bucket to attach that content to . Cheers, -g
Re: svn commit: r1713489 - serf_bucket_readline()
On 11 November 2015 at 19:51, Bert Huijben wrote: >> -Original Message- >> From: Ivan Zhakov [mailto:i...@visualsvn.com] >> Sent: woensdag 11 november 2015 13:22 >> To: Bert Huijben >> Cc: rhuij...@apache.org; dev@serf.apache.org >> Subject: Re: svn commit: r1713489 - serf_bucket_readline() >> >> On 11 November 2015 at 01:44, Bert Huijben wrote: >> >> -Original Message- >> >> From: Bert Huijben [mailto:b...@qqmail.nl] >> >> Sent: dinsdag 10 november 2015 23:37 >> >> To: 'Ivan Zhakov' ; rhuij...@apache.org >> >> Cc: dev@serf.apache.org >> >> Subject: RE: svn commit: r1713489 - in /serf/trunk: >> buckets/event_buckets.c >> >> outgoing.c serf_private.h >> >> > -Original Message- >> >> > From: Ivan Zhakov [mailto:i...@visualsvn.com] >> >> > Sent: dinsdag 10 november 2015 20:43 >> >> > To: rhuij...@apache.org >> >> > Cc: dev@serf.apache.org >> >> > Subject: Re: svn commit: r1713489 - in /serf/trunk: >> >> buckets/event_buckets.c >> >> > outgoing.c serf_private.h >> >> > >> >> > On 9 November 2015 at 20:49, wrote: >> >> > > Author: rhuijben >> >> > > Date: Mon Nov 9 17:49:59 2015 >> >> > > New Revision: 1713489 >> >> > > >> >> > > URL: http://svn.apache.org/viewvc?rev=1713489&view=rev >> >> > > Log: >> >> > > Replace the track bucket in the request writing with a tiny bit more >> >> > > advanced event bucket that tracks both writing done and destroyed. >> The >> >> > > timings of these callbacks will allow simplifying some logic >> >> > > introduced >> >> > > in r1712776. >> >> > > >> >> > > For now declare the event bucket as a private type. >> >> > > >> >> > > * buckets/event_buckets.c >> >> > > New file. >> >> > > >> >> > > * outgoing.c >> >> > > (request_writing_done): New function. >> >> > > (request_writing_finished): Tweak to implement >> >> > serf_bucket_event_callback_t. >> >> > > (write_to_connection): Add event bucket directly after the request >> >> > bucket, >> >> > > instead of an aggregate when the writing is done. >> >> > > >> >> > Hi Bert, >> >> > >> >> > What do you think about alternative design for event buckets: make >> >> > event bucket wrap any other bucket (request bucket in our case)? I >> >> > think it will be more flexible since we could add callback to be >> >> > called before reading from wrapped bucket to track pending request. >> >> >> >> That might work, but would have different characteristics unless you do >> >> more special things. >> > >> > 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. >> > >> How is event bucket situation different from barrier bucket for example? > > The event bucket currently doesn't forward a single read method, while the > barrier forwards all? > I understand that currently event bucket doesn't forward read and other methods. But my suggestion was to make event bucket work differently and wrap existing bucket. -- Ivan Zhakov
RE: svn commit: r1713489 - serf_bucket_readline()
> -Original Message- > From: Ivan Zhakov [mailto:i...@visualsvn.com] > Sent: woensdag 11 november 2015 18:01 > To: Bert Huijben > Cc: rhuij...@apache.org; dev@serf.apache.org > Subject: Re: svn commit: r1713489 - serf_bucket_readline() > > On 11 November 2015 at 19:51, Bert Huijben wrote: > >> -Original Message- > >> From: Ivan Zhakov [mailto:i...@visualsvn.com] > >> Sent: woensdag 11 november 2015 13:22 > >> To: Bert Huijben > >> Cc: rhuij...@apache.org; dev@serf.apache.org > >> Subject: Re: svn commit: r1713489 - serf_bucket_readline() > >> > >> On 11 November 2015 at 01:44, Bert Huijben wrote: > >> >> -Original Message- > >> >> From: Bert Huijben [mailto:b...@qqmail.nl] > >> >> Sent: dinsdag 10 november 2015 23:37 > >> >> To: 'Ivan Zhakov' ; rhuij...@apache.org > >> >> Cc: dev@serf.apache.org > >> >> Subject: RE: svn commit: r1713489 - in /serf/trunk: > >> buckets/event_buckets.c > >> >> outgoing.c serf_private.h > >> >> > -Original Message- > >> >> > From: Ivan Zhakov [mailto:i...@visualsvn.com] > >> >> > Sent: dinsdag 10 november 2015 20:43 > >> >> > To: rhuij...@apache.org > >> >> > Cc: dev@serf.apache.org > >> >> > Subject: Re: svn commit: r1713489 - in /serf/trunk: > >> >> buckets/event_buckets.c > >> >> > outgoing.c serf_private.h > >> >> > > >> >> > On 9 November 2015 at 20:49, wrote: > >> >> > > Author: rhuijben > >> >> > > Date: Mon Nov 9 17:49:59 2015 > >> >> > > New Revision: 1713489 > >> >> > > > >> >> > > URL: http://svn.apache.org/viewvc?rev=1713489&view=rev > >> >> > > Log: > >> >> > > Replace the track bucket in the request writing with a tiny bit more > >> >> > > advanced event bucket that tracks both writing done and > destroyed. > >> The > >> >> > > timings of these callbacks will allow simplifying some logic > introduced > >> >> > > in r1712776. > >> >> > > > >> >> > > For now declare the event bucket as a private type. > >> >> > > > >> >> > > * buckets/event_buckets.c > >> >> > > New file. > >> >> > > > >> >> > > * outgoing.c > >> >> > > (request_writing_done): New function. > >> >> > > (request_writing_finished): Tweak to implement > >> >> > serf_bucket_event_callback_t. > >> >> > > (write_to_connection): Add event bucket directly after the > request > >> >> > bucket, > >> >> > > instead of an aggregate when the writing is done. > >> >> > > > >> >> > Hi Bert, > >> >> > > >> >> > What do you think about alternative design for event buckets: make > >> >> > event bucket wrap any other bucket (request bucket in our case)? I > >> >> > think it will be more flexible since we could add callback to be > >> >> > called before reading from wrapped bucket to track pending > request. > >> >> > >> >> That might work, but would have different characteristics unless you > do > >> >> more special things. > >> > > >> > 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. > >> > > >> How is event bucket situation different from barrier bucket for example? > > > > The event bucket currently doesn't forward a single read method, while > the barrier forwards all? > > > I understand that currently event bucket doesn't forward read and > other methods. But my suggestion was to make event bucket work > differently and wrap existing bucket. I'm starting to think that we should switch to wrapping for the destroy event, but perhaps for a different reason than stated before: The current code depends on implementation details of the aggregate bucket. There are certain cases where it might destroy buckets (or might have destroyed) out of order and when you wrap the bucket you know exactly. Not depending on the knowledge how this bucket works makes this more generic... and better able to handle other aggregation buckets. Bert
Re: svn commit: r1713489 - serf_bucket_readline()
On 14 November 2015 at 12:06, Bert Huijben wrote: >> -Original Message- >> From: Ivan Zhakov [mailto:i...@visualsvn.com] >> Sent: woensdag 11 november 2015 18:01 >> To: Bert Huijben >> Cc: rhuij...@apache.org; dev@serf.apache.org >> Subject: Re: svn commit: r1713489 - serf_bucket_readline() >> >> On 11 November 2015 at 19:51, Bert Huijben wrote: >> >> -Original Message- >> >> From: Ivan Zhakov [mailto:i...@visualsvn.com] >> >> Sent: woensdag 11 november 2015 13:22 >> >> To: Bert Huijben >> >> Cc: rhuij...@apache.org; dev@serf.apache.org >> >> Subject: Re: svn commit: r1713489 - serf_bucket_readline() >> >> >> >> On 11 November 2015 at 01:44, Bert Huijben wrote: >> >> >> -Original Message- >> >> >> From: Bert Huijben [mailto:b...@qqmail.nl] >> >> >> Sent: dinsdag 10 november 2015 23:37 >> >> >> To: 'Ivan Zhakov' ; rhuij...@apache.org >> >> >> Cc: dev@serf.apache.org >> >> >> Subject: RE: svn commit: r1713489 - in /serf/trunk: >> >> buckets/event_buckets.c >> >> >> outgoing.c serf_private.h >> >> >> > -Original Message- >> >> >> > From: Ivan Zhakov [mailto:i...@visualsvn.com] >> >> >> > Sent: dinsdag 10 november 2015 20:43 >> >> >> > To: rhuij...@apache.org >> >> >> > Cc: dev@serf.apache.org >> >> >> > Subject: Re: svn commit: r1713489 - in /serf/trunk: >> >> >> buckets/event_buckets.c >> >> >> > outgoing.c serf_private.h >> >> >> > >> >> >> > On 9 November 2015 at 20:49, wrote: >> >> >> > > Author: rhuijben >> >> >> > > Date: Mon Nov 9 17:49:59 2015 >> >> >> > > New Revision: 1713489 >> >> >> > > >> >> >> > > URL: http://svn.apache.org/viewvc?rev=1713489&view=rev >> >> >> > > Log: >> >> >> > > Replace the track bucket in the request writing with a tiny bit >> >> >> > > more >> >> >> > > advanced event bucket that tracks both writing done and >> destroyed. >> >> The >> >> >> > > timings of these callbacks will allow simplifying some logic >> introduced >> >> >> > > in r1712776. >> >> >> > > >> >> >> > > For now declare the event bucket as a private type. >> >> >> > > >> >> >> > > * buckets/event_buckets.c >> >> >> > > New file. >> >> >> > > >> >> >> > > * outgoing.c >> >> >> > > (request_writing_done): New function. >> >> >> > > (request_writing_finished): Tweak to implement >> >> >> > serf_bucket_event_callback_t. >> >> >> > > (write_to_connection): Add event bucket directly after the >> request >> >> >> > bucket, >> >> >> > > instead of an aggregate when the writing is done. >> >> >> > > >> >> >> > Hi Bert, >> >> >> > >> >> >> > What do you think about alternative design for event buckets: make >> >> >> > event bucket wrap any other bucket (request bucket in our case)? I >> >> >> > think it will be more flexible since we could add callback to be >> >> >> > called before reading from wrapped bucket to track pending >> request. >> >> >> >> >> >> That might work, but would have different characteristics unless you >> do >> >> >> more special things. >> >> > >> >> > 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. >> >> > >> >> How is event bucket situation different from barrier bucket for example? >> > >> > The event bucket currently doesn't forward a single read method, while >> the barrier forwards all? >> > >> I understand that currently event bucket doesn't forward read and >> other methods. But my suggestion was to make event bucket work >> differently and wrap existing bucket. > > I'm starting to think that we should switch to wrapping for the destroy > event, but > perhaps for a different reason than stated before: The current code depends on > implementation details of the aggregate bucket. > > There are certain cases where it might destroy buckets (or might have > destroyed) > out of order and when you wrap the bucket you know exactly. Not depending on > the knowledge how this bucket works makes this more generic... and better able > to handle other aggregation buckets. I thought about the same reason, but missed it in my original email. Sorry. -- Ivan Zhakov
RE: svn commit: r1713489 - serf_bucket_readline()
> -Original Message- > From: Ivan Zhakov [mailto:i...@visualsvn.com] > Sent: zaterdag 14 november 2015 10:19 > To: Bert Huijben > Cc: rhuij...@apache.org; dev@serf.apache.org > Subject: Re: svn commit: r1713489 - serf_bucket_readline() > > > I'm starting to think that we should switch to wrapping for the destroy > event, but > > perhaps for a different reason than stated before: The current code > depends on > > implementation details of the aggregate bucket. > > > > There are certain cases where it might destroy buckets (or might have > destroyed) > > out of order and when you wrap the bucket you know exactly. Not > depending on > > the knowledge how this bucket works makes this more generic... and > better able > > to handle other aggregation buckets. > I thought about the same reason, but missed it in my original email. Sorry. The moment I realized this problem I already thought you did... I just wasn't sure. I'm glad we reached the same conclusion before anything was released as public api. (I didn't make the event buckets public anyway... but perhaps the current code is generic enough to make it public) Now back to coding http/2 request framing... (Which will now use this event bucket instead of a custom solution in yet another bucket type) Bert
limited readline (was: svn commit: r1713489 - serf_bucket_readline())
On Tue, Nov 10, 2015 at 4:44 PM, Bert Huijben 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. 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 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. 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. Cheers, -g
Re: limited readline (was: svn commit: r1713489 - serf_bucket_readline())
And I see you built this in '788. (catching up here) On Wed, Nov 11, 2015 at 10:05 AM, Greg Stein wrote: > On Tue, Nov 10, 2015 at 4:44 PM, Bert Huijben 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. > > 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 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. > > 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. > > Cheers, > -g > >
RE: limited readline (was: svn commit: r1713489 - serf_bucket_readline())
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 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
Re: limited readline (was: svn commit: r1713489 - serf_bucket_readline())
On Wed, Nov 11, 2015 at 11:00 AM, Bert Huijben wrote: >... > > -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()) > >... > > 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
RE: limited readline (was: svn commit: r1713489 - serf_bucket_readline())
[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:gst...@gmail.com] Sent: woensdag 11 november 2015 18:23 To: Bert Huijben 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 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
RE: limited readline (was: svn commit: r1713489 - serf_bucket_readline())
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 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 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
