[jsr369-experts] Re: [servlet-spec users] Re: Re: PushBuilder

From: Mark Thomas <markt_at_apache.org>
Date: Tue, 8 Mar 2016 08:54:09 +0000

On 08/03/2016 05:40, Stuart Douglas 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.

That would work for me.

isPushSupported() can be used to avoid unnecessary work if desired

push() tells them if it actually got sent (if they care)


> 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