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.
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?
Cheers,
Paul
On Fri, Mar 11, 2016 at 2:28 PM, Greg Wilkins <gregw_at_webtide.com> wrote:
>
>
> On 12 March 2016 at 03:19, Edward Burns <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 | office: +1 407 458 0017
>>
>
>
>
> --
> Greg Wilkins <gregw@webtide.com> CTO http://webtide.com
>