> On Mar 12, 2016, at 1:03 AM, Mark Thomas <markt_at_apache.org> wrote:
>
> On 11/03/2016 21:00, Paul Benedict wrote:
>> I think the moral of the story is that we discovered there is no
>> guarantee that a push() entails the client getting the data. So as long
>> as that's documented, I believe getting rid of the "boolean" and
>> restoring "void" makes sense. The return value becomes irrelevant now.
>> Good choice.
>
> The two return values from push() boil down to:
> false - Definitely was not sent
> true - Was sent but the stream might still get reset
>
> I can see that there might be uses for this. Keep in mind the populating
> a PushBuilder is likely to be a relatively expensive process. An
> application might want to monitor how successful the push requests are
> to determine if it is using the best strategy for push.
>
> I'd like to keep the boolean return on push()
>
>> I understand we don't want to throw ISE on a push() if it can't
>> complete. OK. But throwing ISE from getPushBuilder() when
>> isPushSupported() returned false I think can be helpful. That exception
>> tells me there's no possibility of sending data so don't try. For
>> example, the exception can be beneficial to clients who have expensive
>> data retrieval/data preparations for the push; so I like failing them up
>> front. That's a different story to me than the semantics of push()
>> because the client at least was given a weak guarantee there's a
>> possibility of transmitting data.
>>
>> WDYT?
>
> When I suggested adding isPushSupported() I was thinking of apps wanting
> to avoid the expensive process of populating the PushBuilder. I do think
> that we should only throw an ISE if an app does something that is
> clearly wrong.
>
> We seem to be discussing various ways to do the same things.
>
> 1. There needs to be a way for an app to tell early that push isn't
> supported so cycles aren't wasted generating the push. Options are:
> a) do nothing
> b) getPushBuilder() returns null (then the app has to check the return
> value)
> c) isPushSupported()
>
> In order of preference I'd rank these c, b and then (with a big gap) a.
> I think we have broad agreement on this.
>
>
> 2. How to behave if getPushBuilder() is called when push is not
> supported. The race condition complicates this as does whether or not
> isPushSupported() has been called.
> a) return a valid PushBuilder
> b) throw an ISE
> c) throw an ISE iff isPushSupported() previously returned false
> d) return a NO-OP PushBuilder
>
> We could make it a requirement to call isPushSupported() before calling
> getPushBuilder(). But that is really no different to getPushBuilder()
> returning null.
>
> As I type this, the idea of getPushBuilder() returning null if push is
> not supported and not having a separate isPushSupported() method is
> growing on me.
+1 on returning null if push is not supported
Shing Wai Chan
>
> Mark
>
>>
>> Cheers,
>> Paul
>>
>> On Fri, Mar 11, 2016 at 2:28 PM, Greg Wilkins <gregw_at_webtide.com
>> <mailto:gregw_at_webtide.com>> wrote:
>>
>>
>>
>> On 12 March 2016 at 03:19, Edward Burns <edward.burns_at_oracle.com
>> <mailto:edward.burns_at_oracle.com>> wrote:
>>
>> GW> So a simple fix for 2. is that we simply say that
>> isPushSupported always
>> GW> returns false for pushed resources - ie it will always
>> return false when
>> GW> handling B, so B will never attempt to push C. Frameworks
>> will need to do
>> GW> transitive dependencies so the handling of A will push both
>> B and C.
>>
>> With the EDR API I don't even see how this can happen. I thought we
>> abandoned the synthetic GET approach? On HttpServletRequest A
>> we call
>> getPushBuilder(). This returns the PushBuilder that allows us
>> to push
>> B. There is no way in the API to get an HttpServletRequest for
>> B, so
>> there is no way even for there to be a request C. Can you
>> please help
>> me understand?
>>
>>
>>
>> ServletA receives a request.
>> ServletB pushes resource B.
>> ServletB receives the pushed request (which to it looks pretty much
>> the same as a normal request).
>> ServletB pushes resource C.
>>
>>
>> We now have the problem! We are not allowed to push C onto the
>> stream for B by the protocol. If Stream A still exists, we can push
>> C onto A, but that is a race as stream A can complete normally at
>> any time during the handling of ServletB
>>
>> Hence my preference to just say to ServletB that push is not
>> supported. Frameworks will have to adapt and when they receive
>> the request to A, push both B and C (ie the framework has to work
>> out that B depends on C).
>>
>>
>>
>>
>> Here is what we currently have.
>>
>> HttpServletRequest.getPushBuilder
>>
>> Returns a PushBuilder for issuing server push responses from the
>> current request. If the current connection does not support server
>> push, or server push has been disabled by the client via a
>> SETTINGS_ENABLE_PUSH settings frame value of 0 (zero), a valid
>> PushBuilder must still be returned, but calling any of the
>> methods on
>> it will have no effect.
>>
>>
>> Ah I thought we had gone for ISE if somebody called getPushBuilder after
>> isPushSupported returned false.
>>
>> If we have kept the possibility that PushBuilders can be noops, then
>> that is good
>> for avoiding these kinds of races/issues
>>
>>
>>
>>
>> PushBuilder.push()
>>
>> void push()
>>
>> Push a resource given the current state of the builder, returning
>> immediately without blocking.
>>
>> Push a resource based on the current state of the PushBuilder. If
>> isConditional() is true and an etag or lastModified value is
>> provided,
>> then an appropriate conditional header will be generated. If
>> both an
>> etag and lastModified value are provided only an If-None-Match
>> header
>> will be generated. If the builder has a session ID, then the
>> pushed
>> request will include the session ID either as a Cookie or as a URI
>> parameter as appropriate. The builders query string is merged
>> with any
>> passed query string.
>>
>> Before returning from this method, the builder has its path,
>> etag and
>> lastModified fields nulled. All other fields are left as is for
>> possible reuse in another push.
>>
>> Throws:
>>
>> IllegalArgumentException - if the method set expects a
>> request body
>> (eg POST)
>>
>> IllegalStateException - if there was no call to
>> path(java.lang.String) on this instance either between its
>> instantiation or the last call to push() that did not throw an
>> IllegalStateException.
>>
>> The only way I can see us returning a boolean from push() is
>> like this:
>>
>> @returns
>>
>> false if and only if calling isPushSupported() as the first action
>> within the push() implementation returns false. Returns true
>> otherwise.
>>
>> What do you think of that?
>>
>>
>> Actually as PB said, there are places that the race and/or failure
>> can happen even after
>> a true return. Perhaps it is just better to return void, but to
>> note in the javadoc that
>> a push may not happen, or may not be accepted. Ie don't write
>> code that is
>> expecting there will definitely be a request dispatched for each push.
>>
>> cheers
>>
>>
>>
>>
>>
>>
>> Ed
>> --
>> | edward.burns_at_oracle.com <mailto:edward.burns_at_oracle.com> |
>> office: +1 407 458 0017 <tel:%2B1%20407%20458%200017>
>>
>>
>>
>>
>> --
>> Greg Wilkins <gregw_at_webtide.com <mailto:gregw_at_webtide.com>> CTO
>> http://webtide.com
>>
>>
>