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