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

From: Edward Burns <edward.burns_at_oracle.com>
Date: Fri, 11 Mar 2016 08:19:59 -0800

>>>>> On Mon, 15 Feb 2016 11:53:36 +0100, Greg Wilkins <gregw_at_webtide.com> said:

GW> I think there is a small problem with isPushSupported() as it can be a bit
GW> of a race inducing API:

GW> if the API requires code like:

GW> if (request.isPushSupported())
GW> {
GW> request.getPushBuilder()...... push()
GW> }

GW> Then there is a problem is the value of isPushSupported changes after
GW> returning true - ie the getPushBuilder() or push() calls may throw ISE.

GW> There are two cases that we have identified that isPushSupported() can
GW> change value dynamically:

GW> 1. a client may at any time send a setting frame that disables support
GW> for push. This is highly unlikely in reality, but may be part of some kind
GW> of attack if server code is not expecting isPushSupported to change.
GW> 2. If resource A pushes B which attempts to push C, then our ability to
GW> push C depends on the state of A. HTTP2 does not allow nested pushes, ie
GW> we cannot send a push promise for C on B's stream. We can see that B
GW> itself is a push and that it has a parent A. IFF stream A is still in an
GW> appropriate state, then we can send a push_promise for C on A. But the
GW> state of A can change asynchronously with the handling of B, so the ability
GW> to push C is in a race!

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?

GW> I'm not sure how we should handle 1. It makes the throwing of an ISE
GW> from getPushBuilder or push() after an isPushSupported very unlikely but
GW> possible to induce. Perhaps a noop pushbuilder is actually better?

That's what we currently have, see below.

>>>>> On Mon, 7 Mar 2016 21:01:16 +0000, Mark Thomas <markt_at_apache.org> said:

MT> There is always going to be a race of of one form or another for 1. My
MT> concern remains that creating a push request is expensive and apps
MT> should have a way to avoid it when it is known in advance that the push
MT> won't be accepted.

MT> I think the solution is:
MT> - no ISE for getPushBuilder() or push()
MT> - silently NO-OP push()
MT> if the push is not allowed.

>>>>> On Tue, 8 Mar 2016 09:00:38 -0600, Paul Benedict <pbenedict_at_apache.org> said:

PB> I think push() returning a boolean is a good compromise too.

Here is what we currently have.


  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.


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.


    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

The only way I can see us returning a boolean from push() is like this:


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?

| edward.burns_at_oracle.com | office: +1 407 458 0017