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