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