jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1358710 - in /jmeter/trunk: src/protocol/http/org/apache/jmeter/protocol/http/control/CacheManager.java test/src/org/apache/jmeter/protocol/http/control/TestCacheManager.java xdocs/changes.xml
Date Sun, 08 Jul 2012 13:57:50 GMT
On 8 July 2012 14:43, Philippe Mouawad <philippe.mouawad@gmail.com> 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.

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 <philippe.mouawad@gmail.com
>> 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 <sebbaz@gmail.com> wrote:
>>
>>> On 8 July 2012 14:24, Philippe Mouawad <philippe.mouawad@gmail.com>
>>> 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 <sebbaz@gmail.com> wrote:
>>> >
>>> >> On 8 July 2012 14:07, Philippe Mouawad <philippe.mouawad@gmail.com>
>>> 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 <sebbaz@gmail.com> wrote:
>>> >> >
>>> >> >> On 8 July 2012 10:40,  <pmouawad@apache.org> 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.

Mime
View raw message