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
>