jsr369-experts@servlet-spec.java.net

[jsr369-experts] Re: [servlet-spec users] Re: Re: Re: Re: Re: Re: Re: PushBuilder

From: Greg Wilkins <gregw_at_webtide.com>
Date: Sat, 12 Mar 2016 07:18:36 +1100

Paul,

I tend to agree that I can see little use for the returned boolean, but it
is cheap/easy to implement and low impact.
I'm happy with a noop in the case of a race, but I'm equally happy to
return a false. I'm not keen on throwing an ISE.

cheers

On 11 March 2016 at 13:35, Paul Benedict <pbenedict_at_apache.org> wrote:

> IMO, if the servlet container pushed the data, whether the browser
> accepted it or not, it's considered a success in my eyes. You are right and
> I agree the Javadoc should state that doesn't imply the client actually got
> the data. That's an important distinction. Good catch.
>
> So I think there is a difference. I don't think we should treat pushing
> bytes in the builder any morally different than file or socket writing.
> That's why I advocated for ISE in the beginning (and still don't have an
> issue with it), but a boolean is better than not knowing if the container
> was able to honor the push. Just my two cents.
> On Mar 10, 2016 8:07 PM, "Stuart Douglas" <sdouglas_at_redhat.com> wrote:
>
>> Something we should probably note in the Javadoc is that even if .push()
>> returns true it is no guarantee that the browser will accept it (i.e. it
>> could still just reset the stream).
>>
>> It is for this reason that I don't think this is a super useful API, its
>> the difference between 'rejected because there was a race with a settings
>> frame' (which seems very unlikely to actually happen in the real world) vs
>> 'rejected due to RST_STREAM' (which IMHO is much more likely to happen).
>>
>> Stuart
>>
>> ----- Original Message -----
>> > From: "Greg Wilkins" <gregw_at_webtide.com>
>> > To: jsr369-experts_at_servlet-spec.java.net
>> > Sent: Friday, 11 March, 2016 12:51:38 PM
>> > Subject: [servlet-spec users] [jsr369-experts] Re: Re: Re: Re: Re:
>> PushBuilder
>> >
>> > push() returning a boolean is good for me too!
>> >
>> >
>> > On 9 March 2016 at 02:00, Paul Benedict <pbenedict_at_apache.org> wrote:
>> >
>> > > I think push() returning a boolean is a good compromise too.
>> > >
>> > > Cheers,
>> > > Paul
>> > >
>> > > On Mon, Mar 7, 2016 at 11:40 PM, Stuart Douglas <sdouglas_at_redhat.com>
>> > > wrote:
>> > >
>> > >>
>> > >>
>> > >> ----- Original Message -----
>> > >> > From: "Paul Benedict" <pbenedict_at_apache.org>
>> > >> > To: jsr369-experts_at_servlet-spec.java.net
>> > >> > Sent: Tuesday, 8 March, 2016 8:10:01 AM
>> > >> > Subject: [jsr369-experts] Re: [servlet-spec users] Re: Re:
>> PushBuilder
>> > >> >
>> > >> > Personally, I think ISE is always preferable to silently doing
>> nothing.
>> > >> > Otherwise, the contract of the method needs to change so that
>> users know
>> > >> > it's permissible to discard the operation, in essence, because
>> push()
>> > >> has
>> > >> > the chance to do nothing. I'd rather know of the illegal state.
>> > >> >
>> > >> > Cheers,
>> > >> > Paul
>> > >>
>> > >> This would essentially mean that from a users point of view that no
>> > >> matter what they do this method may sometimes throw an ISE, which I
>> don't
>> > >> think is a great API.
>> > >>
>> > >> I think using the return value of the push() method to determine if
>> the
>> > >> push suceeded would be better, as in general the user code should not
>> > >> really care if the push succeeds or fails by this point anyway.
>> > >>
>> > >> Stuart
>> > >>
>> > >>
>> > >> >
>> > >> > On Mon, Mar 7, 2016 at 3:01 PM, Mark Thomas <markt_at_apache.org>
>> wrote:
>> > >> >
>> > >> > > On 15/02/2016 10:53, Greg Wilkins wrote:
>> > >> > > >
>> > >> > > > I think there is a small problem with isPushSupported() as it
>> can
>> > >> be a
>> > >> > > > bit of a race inducing API:
>> > >> > > >
>> > >> > > > if the API requires code like:
>> > >> > > >
>> > >> > > > if (request.isPushSupported())
>> > >> > > > {
>> > >> > > > request.getPushBuilder()...... push()
>> > >> > > > }
>> > >> > > >
>> > >> > > >
>> > >> > > > Then there is a problem is the value of isPushSupported changes
>> > >> after
>> > >> > > > returning true - ie the getPushBuilder() or push() calls may
>> throw
>> > >> ISE.
>> > >> > > >
>> > >> > > > There are two cases that we have identified that
>> isPushSupported()
>> > >> can
>> > >> > > > change value dynamically:
>> > >> > > >
>> > >> > > > 1. a client may at any time send a setting frame that disables
>> > >> support
>> > >> > > > for push. This is highly unlikely in reality, but may be
>> part
>> > >> of
>> > >> > > > some kind of attack if server code is not expecting
>> > >> isPushSupported
>> > >> > > > to change.
>> > >> > > > 2. If resource A pushes B which attempts to push C, then our
>> > >> ability to
>> > >> > > > push C depends on the state of A. HTTP2 does not allow
>> nested
>> > >> > > > pushes, ie we cannot send a push promise for C on B's
>> stream.
>> > >> We
>> > >> > > > can see that B itself is a push and that it has a parent
>> A. IFF
>> > >> > > > stream A is still in an appropriate state, then we can
>> send a
>> > >> > > > push_promise for C on A. But the state of A can change
>> > >> > > > asynchronously with the handling of B, so the ability to
>> push C
>> > >> is
>> > >> > > > in a race!
>> > >> > > >
>> > >> > > > So a simple fix for 2. is that we simply say that
>> isPushSupported
>> > >> always
>> > >> > > > returns false for pushed resources - ie it will always return
>> false
>> > >> when
>> > >> > > > handling B, so B will never attempt to push C. Frameworks
>> will
>> > >> need to
>> > >> > > > do transitive dependencies so the handling of A will push both
>> B
>> > >> and C.
>> > >> > > >
>> > >> > > > I'm not sure how we should handle 1. It makes the throwing
>> of an
>> > >> ISE
>> > >> > > > from getPushBuilder or push() after an isPushSupported very
>> > >> unlikely but
>> > >> > > > possible to induce. Perhaps a noop pushbuilder is actually
>> better?
>> > >> > > >
>> > >> > > > thoughts?
>> > >> > >
>> > >> > > There is always going to be a race of of one form or another for
>> 1. My
>> > >> > > concern remains that creating a push request is expensive and
>> apps
>> > >> > > should have a way to avoid it when it is known in advance that
>> the
>> > >> push
>> > >> > > won't be accepted.
>> > >> > >
>> > >> > > I think the solution is:
>> > >> > > - no ISE for getPushBuilder() or push()
>> > >> > > - silently NO-OP push()
>> > >> > > if the push is not allowed.
>> > >> > >
>> > >> > > Mark
>> > >> > >
>> > >> >
>> > >>
>> > >
>> > >
>> >
>> >
>> > --
>> > Greg Wilkins <gregw@webtide.com> CTO http://webtide.com
>> >
>>
>


-- 
Greg Wilkins <gregw@webtide.com> CTO http://webtide.com