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
> >
>