Issue Details (XML | Word | Printable)

Key: JAVASERVERFACES-1557
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Critical Critical
Assignee: Ryan Lubke
Reporter: Frank Caputo
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
javaserverfaces

ResourceHandlerImpl does not cache in production mode

Created: 18/Feb/10 05:34 AM   Updated: 01/Mar/12 03:08 PM   Resolved: 02/Mar/10 10:50 AM
Component/s: resources
Affects Version/s: 2.0.2
Fix Version/s: 2.0.3-b01

Time Tracking:
Not Specified

File Attachments: 1. Text File changebundle.txt (18 kB) 02/Mar/10 10:42 AM - Ryan Lubke

Environment:

Operating System: All
Platform: All


Issuezilla Id: 1,557
Tags:
Participants: Ed Burns, Frank Caputo, Jason Lee, Manfred Riem and Ryan Lubke


 Description  « Hide

ResourceHandlerImpl.handleResourceRequest takes too long to deliver a 304. From
my point of view, delivering a 304 should take quasi no time.

The problem seems to be that ResourceManager.findLibrary is called again and
again. Can resources really change in production mode?

If resources can't change in production mode, simply cache all necessary
information (most notably last modified) and return 304 immediately.

I will look deeper into this. Maybe i will find some point to change.



Ryan Lubke added a comment - 18/Feb/10 09:10 AM

Resources can change in production mode. If you don't plan on having them
change, then set the context init parameter
'com.sun.faces.resourceUpdateCheckPeriod' to -1.

Regardless, the library will always have to be looked up (assuming the library
name was included with the resource url) with the potential of a cached result
returned.


Ed Burns added a comment - 20/Feb/10 04:48 AM

I have observed this as well. It's a problem.


Frank Caputo added a comment - 22/Feb/10 03:54 AM

Will you fix it?


Ed Burns added a comment - 22/Feb/10 08:16 AM

Before we go further, if you set 'com.sun.faces.resourceUpdateCheckPeriod' to -1,
is the performance acceptable?


Ryan Lubke added a comment - 22/Feb/10 11:08 AM

Assigned.


Ryan Lubke added a comment - 23/Feb/10 10:36 AM

Guess I'm not seeing an issue here.

The 304 response is independent of ResourceManager interactions. Based on the
API, the Resource has to be looked up before we can determine if the user agent
needs update.

This means we have to go through the ResourceManager to obtain the cached
metadata. Note that by default, the metadata will be refreshed every 5 minutes
from its last access unless the configuration option I referenced previously is
set to -1.

However, anytime the user agent asks about the modified time of a resource,
we'll answer it.

Doing some local testing with FF 3.6 and Live HTTP Headers, if I make an initial
request to an application with managed resources, I see the GET being made to
the resources with the Expires, Last-Modified, and ETag response headers being
sent.

If I then issue the same GET request, I see only the GET for the page, and not
the resources, so the conditional GET sequence is not initiated. Assuming no
reloads, the request for the resource shouldn't be issues again for another
seven days.

If the reload button is hit, then a request is sent for the resources with an
If-Modified-Since request header. This cases
ResourceImpl.userAgentNeedsUpdate() to be invoked which will query the timestamp.

If a shift-reload occurs, then the resource is re-requested with no
If-Modified-Since header.

If you're seeing the conditional GET requests being sent for every resource
regardless of the headers, I'd like to know what user agent you're using.


Frank Caputo added a comment - 24/Feb/10 03:02 AM

I also have no problem, if firefox doesn't ask for the resources. But if he
asks, delivering a 304 takes too long, even if i set the check period to -1.

The problem is obviously that ResourceHelper.getLastModified always opens a
connection to get the timestamp and makes unnecessary IO even if i disable
resource refreshing.

If resource refreshing is disabled, you could simply remember the last modified
date in the ResourceInfo and return it immediately without doing IO.

I think this would help a lot.


Ryan Lubke added a comment - 02/Mar/10 10:42 AM

Created an attachment (id=1133)
Proposed changes (ver 1)


Jason Lee added a comment - 02/Mar/10 10:46 AM

r=jdlee


Ryan Lubke added a comment - 02/Mar/10 10:50 AM

Changes applied (r8352).

We'll cache the timestamp for the lifetime if the ResourceInfo instance,
however, the cache behavior is disabled by default. The cache behavior can
be enabled by setting the com.sun.faces.cacheResourceModificationTimestamp
context init parameter to true.


Ryan Lubke added a comment - 02/Mar/10 10:50 AM

Mark fixed in b01.


Manfred Riem added a comment - 01/Mar/12 03:08 PM

Closing issue out