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