Jetty respond with status 200 instead of 304 while using Servlet 4.0 PushBuilder #801

Closed
klopfdreh opened this Issue Aug 2, 2016 · 12 comments

Projects

None yet

2 participants

@klopfdreh
klopfdreh commented Aug 2, 2016 edited

Because the bug tracking system says:

Sorry, entering a bug into the product Jetty has been disabled.

I just want to file in the issue this way.

Summary:

I think the server (jetty 9.3.9-M1) does not accept the RST_STREAM command after the PUSH_PROMISE has been sent, because the client status code is always 200 instead of 304 for the pushed resource after the second request to the index page. So the resource is always pushed instead the client side cached one is used.

More information and a test case:

@klopfdreh klopfdreh changed the title from Jetty respond with status 200 instead of 304 while using http2 to Jetty respond with status 200 instead of 304 while using Servlet 4.0 PushBuilder Aug 4, 2016
@sbordet sbordet self-assigned this Aug 9, 2016
@sbordet
Contributor
sbordet commented Aug 9, 2016 edited

Cannot reproduce. It's not clear what exactly the problem is, neither here nor in the StackOverflow page.

A resource that is being pushed always has a response code of 200.

The browser controls whether the server responds with a 304 (or pushes) by sending the conditional headers such as If-Modified-Since.
If the browser does not send conditional headers, then Jetty will reply with a 200 (or push).
If the browser sends conditional headers, then Jetty will reply with a 304 (or avoid pushing).

Closing this issue.

If you have more information, we need exact reproducible steps, what is your expectation and what you get instead.

Thanks !

@sbordet sbordet closed this Aug 9, 2016
@klopfdreh
klopfdreh commented Aug 9, 2016 edited

Hi @sbordet,

The browser controls whether the server responds with a 304 (or pushes) by sending the conditional headers such as If-Modified-Since.

That is exactly what is not happening in my demo project I linked in the issue.

After the second request the server should not send the resource again with 200 (push), but for me it does.

It should avoid pushing with a 304.

Here is a screenshot of the second request to the index page. The server pushes the resource again.

bildschirmfoto 2016-08-09 um 18 47 21

Also Chrome is listing a push:

bildschirmfoto 2016-08-09 um 18 51 56

Maybe there is something wrong with the request headers to avoid pushing again?

kind regards and thanks a lot for your time!

Tobias

The steps to reproduce it:

  1. Clone the git repository: https://github.com/klopfdreh/jetty-http2-example
  2. Follow the steps here: https://github.com/klopfdreh/jetty-http2-example/blob/master/README.md
  3. Access the server with https://127.0.0.1:8443
@sbordet
Contributor
sbordet commented Aug 10, 2016

@klopfdreh I think there is a misunderstanding of what the push APIs do. Right now, they just push.

The logic of whether the request is conditional (i.e. contains headers such as If-Modified-Since) it is not inside the PushBuilder implementation, it should be in application code.

See for example how it has been implemented in Jetty's https://github.com/eclipse/jetty.project/blob/jetty-9.3.11.v20160721/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/PushCacheFilter.java[PushCacheFilter].

You should implement this logic in your PushHeaderItem.render().

If you do, can you report back if it worked as expected ?

Thanks !

@klopfdreh

Hi @sbordet ,

first of all thanks for the clarification - great to get help in this issue here!

The implementation of Apache Wicket in this case is rather easy. The PushHeaderItem is created when the initial request to index page is going to be performed at this place:

https://github.com/klopfdreh/jetty-http2-example/blob/4160d7ac393b0a8d85a59671968572148a566abe/de.jetty.wicket.http2.example/src/main/java/de/jetty/wicket/http2/example/HTTP2Page.java#L28

At this time I don't know if the client is going to send the corresponding header for the resource (If-Modified-Since), because it is the request for the index page.

So if the response of the index page is send to the client and it does not received the resource previously via push (in the renderHead) I thought the client is going to request the resource as usual without the benefit of the push API, because it reads the link tag and perform an additional request.

To visualize

With PushBuilder
-> index request -> resource via push -> index response

Without PushBuilder
-> index request -> index response -> resource request -> resource response

I hope I could explain my trouble with it.

kind regards

Tobias

@sbordet
Contributor
sbordet commented Aug 10, 2016 edited

@klopfdreh I don't think you have this right.

The first time the client requests index.html without conditional headers and your application, on server, knows that it also needs styles.css, so it pushes it.

The second time, the client requests index.html with the conditional header, so your application infers that because the primary resource has been requested conditionally, then also secondary resources (in this case styles.css) were served before and they are probably still valid in the browser cache, so it does not push them.

The Link header is not read by clients, so it is out of the picture here.

The Link header would be read by Jetty when you return index.html with a Link response header, so that Jetty could push styles.css on your behalf (while now you are pushing it manually using PushBuilder).

I don't know Wicket, but the logic to check conditional headers should be put in PushHeaderItem.render(), assuming that this method is called for every request to main page (index.html in the example above). There, you have access to the request, you can check the headers, you can push (or not) with your logic.

@klopfdreh

Hi @sbordet ,

now I see - it all depends of the index page - even if the resources are pushed.

Now it is clear what we have to implement to achieve the expected behavior.

Thanks a lot!

kind regards

Tobias

@klopfdreh
klopfdreh commented Aug 11, 2016 edited

Hi @sbordet ,

I changed the code in that way:

This results in the following behavior:

  • When a browser requests the page with an initial request everything is going to be pushed with (200)
  • When the browser requests the page a second time resources are not pushed (304) not modified, because of the actual ResourceReferences headers
  • When the browser requests the page a second time and the markup of the page has changed everything is going to be pushed again (200)
  • When the browser requests the page a second time and resource references has been changed but not the page markup, all changed resource references are shipped via separate requests

So the framework decides now when to push the resource and when it is better to save bandwidth. What do you think about that solution?

Anyway thanks a lot for the great help! If this implementation is ok I am going to apply the changes to the experimental project of http://wicket.apache.org/ and update the documentation https://ci.apache.org/projects/wicket/guide/8.x/guide/http2push.html soon.

kind regards

Tobias

Edit: apache/wicket@76116aa

@sbordet
Contributor
sbordet commented Aug 11, 2016

@klopfdreh looks good to me.

Out of curiosity, and related to #193, would you prefer to add a Link header and have Jetty doing the push on your behalf, rather than you pushing explicitly ?

You would maintain the logic related to If-Modified-Since, but just add a Link: <styles.css> rel=preload. Jetty will do the push for you.

@klopfdreh

Hi @sbordet ,

is this preload also working for a response composed via the servlet output stream? I ask because the response is no static resource file. Apache Wicket aggregates html files / the corresponding classes and the header items are rendered dynamically into the aggregated content.

Another question: How is PushBuilder / preload working together - does the server ensure that the resource is not pushed twice?

kind regards and thanks for having a look at the implementation.

Tobias

@sbordet
Contributor
sbordet commented Aug 11, 2016

Note that this is not yet implemented (hence #193).

Wicket will aggregate secondary resources, but eventually it will send just one response per primary resource. The headers on that response are important, there you want to add the Link headers. You want to do this before any content is written to the response output stream, because the first content may cause the response to be committed (and the headers sent to the client, so cannot be modified anymore).

How the actual response content is composed by Wicket would be irrelevant, only the headers matter.

@klopfdreh
klopfdreh commented Aug 11, 2016 edited

I had a look at the existing classes and it could be implemented with this class:

https://ci.apache.org/projects/wicket/apidocs/8.x/index.html?org/apache/wicket/markup/head/MetaDataHeaderItem.html

MetaDataHeaderItem.forLinkTag("preload",<url>); instead of CssHeaderItem

and the URL can be rendered with:

RequestCycle.get().urlFor(<resourcereference>);

@klopfdreh

This should be no problem then, because Wicket commits the response if everything is aggregated as far as I know. I am going to add this to the user guide, too.

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment