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:58:56 GMT
On 8 July 2012 14:57, sebb <sebbaz@gmail.com> wrote:
> 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.

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 <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