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.
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
>
>