On 8 July 2012 14:57, sebb <[email protected]> wrote:
> On 8 July 2012 14:43, Philippe Mouawad <[email protected]> wrote:
>> We would have this:
>>
>> private void setCache(String lastModified, String cacheControl, String
>> expires, String etag, String url) {
>> if (log.isDebugEnabled()){
>> log.debug("SET(both) "+url + " " + cacheControl + " " +
>> lastModified + " " + " " + expires + " " + etag);
>> }
>> Date expiresDate = null; // i.e. not using Expires
>> if (useExpires) {// Check that we are processing
>> Expires/CacheControl
>> final String MAX_AGE = "max-age=";
>> if(cacheControl != null) {
>> if(cacheControl.contains("no-cache")) {
>> return;
>> }
>
> Surely that should be done before checking useExpires?
>
>> if(cacheControl.contains(MAX_AGE)) {
>
> cacheControl could be null here.
Sorry, misread conditional nesting - ignore that.
> Could fix this by changing
>
> if(useExpires) to if(useExpires && cacheControl != null)
>
> We also need to change component_reference now that public is not
> required for max-age.
>
>> long maxAgeInSecs = Long.parseLong(
>>
>> cacheControl.substring(cacheControl.indexOf(MAX_AGE)+MAX_AGE.length())
>> .split("[, ]")[0] // Bug 51932 - allow for
>> optional trailing attributes
>> );
>> expiresDate=new
>> Date(System.currentTimeMillis()+maxAgeInSecs*1000);
>> }
>> } else if (expires != null) {
>> try {
>> expiresDate = DateUtil.parseDate(expires);
>> } catch (DateParseException e) {
>> if (log.isDebugEnabled()){
>> log.debug("Unable to parse Expires: '"+expires+"'
>> "+e);
>> }
>> expiresDate = new Date(0L); // invalid dates must be
>> treated as expired
>> }
>> }
>> }
>> getCache().put(url, new CacheEntry(lastModified, expiresDate,
>> etag));
>> }
>>
>> On Sun, Jul 8, 2012 at 3:36 PM, Philippe Mouawad <[email protected]
>>> wrote:
>>
>>> Looking at existing code, I noticed that with no-cache we store an entry
>>> in Cache for which CacheManager#inCache will return false .
>>>
>>> I don't understand why we just skip the entry, currently we use on entry
>>> in map for nothing.
>>>
>>> So reading what you suggest + this I propose we change to the following:
>>>
>>> - We test no-cache or no-store and if true we just return
>>> - we remove check on (cacheControl.contains("public") ||
>>> cacheControl.contains("private"))
>>>
>>>
>>> Agree ?
>>>
>>> On Sun, Jul 8, 2012 at 3:29 PM, sebb <[email protected]> wrote:
>>>
>>>> On 8 July 2012 14:24, Philippe Mouawad <[email protected]>
>>>> wrote:
>>>> > but if we have this:
>>>> > Cache-Control=no-cache, max-age=10.
>>>> >
>>>> > If we don't check we will cache which is wrong right ?
>>>> > This header is wrong but I have already seen this on tests I did.
>>>> >
>>>> > Or I am misunderstanding ?
>>>>
>>>> I don't have the source to hand at present, but we should not cache at
>>>> all if Cache-Control=no-cache; the max-age is then not relevant.
>>>>
>>>> > Regards
>>>> > Philippe
>>>> >
>>>> > On Sun, Jul 8, 2012 at 3:19 PM, sebb <[email protected]> wrote:
>>>> >
>>>> >> On 8 July 2012 14:07, Philippe Mouawad <[email protected]>
>>>> wrote:
>>>> >> > Can't it also be no-cache ? no-store ?
>>>> >> > If we don't check it , we could store in cache if server returns
>>>> invalid
>>>> >> > headers no ?
>>>> >>
>>>> >> What do we do if we don't check MaxAge?
>>>> >>
>>>> >> I don't think we should be more restrictive just because we are also
>>>> >> checking the age.
>>>> >>
>>>> >> >
>>>> >> > Thansk for looking at 53365.
>>>> >> > Regards
>>>> >> > Philippe
>>>> >> >
>>>> >> > On Sun, Jul 8, 2012 at 3:01 PM, sebb <[email protected]> wrote:
>>>> >> >
>>>> >> >> On 8 July 2012 10:40, <[email protected]> wrote:
>>>> >> >> > Author: pmouawad
>>>> >> >> > Date: Sun Jul 8 09:40:51 2012
>>>> >> >> > New Revision: 1358710
>>>> >> >> >
>>>> >> >> > URL: http://svn.apache.org/viewvc?rev=1358710&view=rev
>>>> >> >> > Log:
>>>> >> >> > Bug 53521 - Cache Manager should cache content with
>>>> >> Cache-control=private
>>>> >> >> > Bugzilla Id: 53521
>>>> >> >> >
>>>> >> >> > Modified:
>>>> >> >> >
>>>> >> >>
>>>> >>
>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java
>>>> >> >> >
>>>> >> >>
>>>> >>
>>>> jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java
>>>> >> >> > jmeter/trunk/xdocs/changes.xml
>>>> >> >> >
>>>> >> >> > Modified:
>>>> >> >>
>>>> >>
>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java
>>>> >> >> > URL:
>>>> >> >>
>>>> >>
>>>> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java?rev=1358710&r1=1358709&r2=1358710&view=diff
>>>> >> >> >
>>>> >> >>
>>>> >>
>>>> ==============================================================================
>>>> >> >> > ---
>>>> >> >>
>>>> >>
>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java
>>>> >> >> (original)
>>>> >> >> > +++
>>>> >> >>
>>>> >>
>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java
>>>> >> >> Sun Jul 8 09:40:51 2012
>>>> >> >> > @@ -161,7 +161,7 @@ public class CacheManager extends Config
>>>> >> >> > if (useExpires) {// Check that we are processing
>>>> >> >> Expires/CacheControl
>>>> >> >> > final String MAX_AGE = "max-age=";
>>>> >> >> > // TODO - check for other CacheControl attributes?
>>>> >> >> > - if (cacheControl != null &&
>>>> >> cacheControl.contains("public")
>>>> >> >> && cacheControl.contains(MAX_AGE)) {
>>>> >> >> > + if (cacheControl != null &&
>>>> >> >> (cacheControl.contains("public") ||
>>>> cacheControl.contains("private")) &&
>>>> >> >> cacheControl.contains(MAX_AGE)) {
>>>> >> >>
>>>> >> >> Not sure this is the correct fix.
>>>> >> >> Do we really care if the string "public" or "private" is present so
>>>> >> >> long as there is a MAX_AGE entry?
>>>> >> >> We could just drop the check for "public" instead.
>>>> >> >>
>>>> >> >> > long maxAgeInSecs = Long.parseLong(
>>>> >> >> >
>>>> >> >>
>>>> cacheControl.substring(cacheControl.indexOf(MAX_AGE)+MAX_AGE.length())
>>>> >> >> > .split("[, ]")[0] // Bug 51932 -
>>>> allow
>>>> >> for
>>>> >> >> optional trailing attributes
>>>> >> >> >
>>>> >> >> > Modified:
>>>> >> >>
>>>> >>
>>>> jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java
>>>> >> >> > URL:
>>>> >> >>
>>>> >>
>>>> http://svn.apache.org/viewvc/jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java?rev=1358710&r1=1358709&r2=1358710&view=diff
>>>> >> >> >
>>>> >> >>
>>>> >>
>>>> ==============================================================================
>>>> >> >> > ---
>>>> >> >>
>>>> >>
>>>> jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java
>>>> >> >> (original)
>>>> >> >> > +++
>>>> >> >>
>>>> >>
>>>> jmeter/trunk/test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java
>>>> >> >> Sun Jul 8 09:40:51 2012
>>>> >> >> > @@ -238,7 +238,30 @@ public class TestCacheManager extends JM
>>>> >> >> > assertNotNull("Should find
>>>> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
>>>> >> >> > assertTrue("Should find valid
>>>> >> >> entry",this.cacheManager.inCache(url));
>>>> >> >> > }
>>>> >> >> > +
>>>> >> >> > + public void testPrivateCacheHttpClient() throws Exception{
>>>> >> >> > + this.cacheManager.setUseExpires(true);
>>>> >> >> > + this.cacheManager.testIterationStart(null);
>>>> >> >> > + assertNull("Should not find
>>>> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
>>>> >> >> > + assertFalse("Should not find valid
>>>> >> >> entry",this.cacheManager.inCache(url));
>>>> >> >> > + ((HttpMethodStub)httpMethod).expires=makeDate(new
>>>> >> >> Date(System.currentTimeMillis()));
>>>> >> >> > + ((HttpMethodStub)httpMethod).cacheControl="private,
>>>> >> max-age=10";
>>>> >> >> > + this.cacheManager.saveDetails(httpMethod,
>>>> sampleResultOK);
>>>> >> >> > + assertNotNull("Should find
>>>> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
>>>> >> >> > + assertTrue("Should find valid
>>>> >> >> entry",this.cacheManager.inCache(url));
>>>> >> >> > + }
>>>> >> >> >
>>>> >> >> > + public void testNoCacheHttpClient() throws Exception{
>>>> >> >> > + this.cacheManager.setUseExpires(true);
>>>> >> >> > + this.cacheManager.testIterationStart(null);
>>>> >> >> > + assertNull("Should not find
>>>> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
>>>> >> >> > + assertFalse("Should not find valid
>>>> >> >> entry",this.cacheManager.inCache(url));
>>>> >> >> > + ((HttpMethodStub)httpMethod).cacheControl="no-cache";
>>>> >> >> > + this.cacheManager.saveDetails(httpMethod,
>>>> sampleResultOK);
>>>> >> >> > + assertNotNull("Should find
>>>> >> >> entry",getThreadCacheEntry(LOCAL_HOST));
>>>> >> >> > + assertFalse("Should not find valid
>>>> >> >> entry",this.cacheManager.inCache(url));
>>>> >> >> > + }
>>>> >> >> > +
>>>> >> >> > public void testCacheHttpClientBug51932() throws Exception{
>>>> >> >> > this.cacheManager.setUseExpires(true);
>>>> >> >> > this.cacheManager.testIterationStart(null);
>>>> >> >> >
>>>> >> >> > Modified: jmeter/trunk/xdocs/changes.xml
>>>> >> >> > URL:
>>>> >> >>
>>>> >>
>>>> http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1358710&r1=1358709&r2=1358710&view=diff
>>>> >> >> >
>>>> >> >>
>>>> >>
>>>> ==============================================================================
>>>> >> >> > --- jmeter/trunk/xdocs/changes.xml (original)
>>>> >> >> > +++ jmeter/trunk/xdocs/changes.xml Sun Jul 8 09:40:51 2012
>>>> >> >> > @@ -59,6 +59,7 @@ or a Debug Sampler with all fields set t
>>>> >> >> >
>>>> >> >> > <h3>HTTP Samplers and Proxy</h3>
>>>> >> >> > <ul>
>>>> >> >> > +<li><bugzilla>53521</bugzilla> - Cache Manager should cache
>>>> content
>>>> >> >> with Cache-control=private</li>
>>>> >> >> > </ul>
>>>> >> >> >
>>>> >> >> > <h3>Other Samplers</h3>
>>>> >> >> >
>>>> >> >> >
>>>> >> >>
>>>> >> >
>>>> >> >
>>>> >> >
>>>> >> > --
>>>> >> > Cordialement.
>>>> >> > Philippe Mouawad.
>>>> >>
>>>> >
>>>> >
>>>> >
>>>> > --
>>>> > Cordialement.
>>>> > Philippe Mouawad.
>>>>
>>>
>>>
>>>
>>> --
>>> Cordialement.
>>> Philippe Mouawad.
>>>
>>>
>>>
>>>
>>
>>
>> --
>> Cordialement.
>> Philippe Mouawad.
