From 161a14d9d0b3d4d4f8f744f2171c834ed47399ad Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 24 Jan 2013 14:07:25 +1100 Subject: [PATCH] 381521 Only set Vary header when content could be compressed --- .../http/gzip/AbstractCompressedStream.java | 54 +++++--- .../http/gzip/CompressedResponseWrapper.java | 4 +- .../jetty/server/handler/GzipHandler.java | 28 +++- .../eclipse/jetty/servlets/GzipFilter.java | 127 ++++++++++-------- .../jetty/servlets/IncludableGzipFilter.java | 33 ++++- .../jetty/servlets/gzip/GzipTester.java | 3 +- 6 files changed, 165 insertions(+), 84 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/gzip/AbstractCompressedStream.java b/jetty-http/src/main/java/org/eclipse/jetty/http/gzip/AbstractCompressedStream.java index 8787d5efc92..8317914d9ce 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/gzip/AbstractCompressedStream.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/gzip/AbstractCompressedStream.java @@ -40,6 +40,7 @@ import org.eclipse.jetty.util.ByteArrayOutputStream2; public abstract class AbstractCompressedStream extends ServletOutputStream { private final String _encoding; + protected final String _vary; protected final CompressedResponseWrapper _wrapper; protected final HttpServletResponse _response; protected OutputStream _out; @@ -52,12 +53,13 @@ public abstract class AbstractCompressedStream extends ServletOutputStream * Instantiates a new compressed stream. * */ - public AbstractCompressedStream(String encoding,HttpServletRequest request, CompressedResponseWrapper wrapper) + public AbstractCompressedStream(String encoding,HttpServletRequest request, CompressedResponseWrapper wrapper,String vary) throws IOException { _encoding=encoding; _wrapper = wrapper; _response = (HttpServletResponse)wrapper.getResponse(); + _vary=vary; if (_wrapper.getMinCompressSize()==0) doCompress(); @@ -106,7 +108,7 @@ public abstract class AbstractCompressedStream extends ServletOutputStream { long length=_wrapper.getContentLength(); if (length > 0 && length < _wrapper.getMinCompressSize()) - doNotCompress(); + doNotCompress(false); else doCompress(); } @@ -137,13 +139,14 @@ public abstract class AbstractCompressedStream extends ServletOutputStream _wrapper.setContentLength(length); } if (length < _wrapper.getMinCompressSize()) - doNotCompress(); + doNotCompress(false); else doCompress(); } else if (_out == null) { - doNotCompress(); + // No output + doNotCompress(false); } if (_compressedOutputStream != null) @@ -168,7 +171,7 @@ public abstract class AbstractCompressedStream extends ServletOutputStream { long length=_wrapper.getContentLength(); if (length > 0 && length < _wrapper.getMinCompressSize()) - doNotCompress(); + doNotCompress(false); else doCompress(); } @@ -226,23 +229,30 @@ public abstract class AbstractCompressedStream extends ServletOutputStream if (_response.isCommitted()) throw new IllegalStateException(); - setHeader("Content-Encoding", _encoding); - if (_response.containsHeader("Content-Encoding")) + if (_encoding!=null) { - _out=_compressedOutputStream=createStream(); - - if (_bOut!=null) + setHeader("Content-Encoding", _encoding); + if (_response.containsHeader("Content-Encoding")) { - _out.write(_bOut.getBuf(),0,_bOut.getCount()); - _bOut=null; + setHeader("Vary",_vary); + _out=_compressedOutputStream=createStream(); + if (_out!=null) + { + if (_bOut!=null) + { + _out.write(_bOut.getBuf(),0,_bOut.getCount()); + _bOut=null; + } + + String etag=_wrapper.getETag(); + if (etag!=null) + setHeader("ETag",etag.substring(0,etag.length()-1)+'-'+_encoding+'"'); + return; + } } - - String etag=_wrapper.getETag(); - if (etag!=null) - setHeader("ETag",etag.substring(0,etag.length()-1)+'-'+_encoding+'"'); } - else - doNotCompress(); + + doNotCompress(true); // Send vary as it could have been compressed if encoding was present } } @@ -252,12 +262,14 @@ public abstract class AbstractCompressedStream extends ServletOutputStream * @throws IOException * Signals that an I/O exception has occurred. */ - public void doNotCompress() throws IOException + public void doNotCompress(boolean sendVary) throws IOException { if (_compressedOutputStream != null) throw new IllegalStateException("Compressed output stream is already assigned."); if (_out == null || _bOut != null) { + if (sendVary) + setHeader("Vary",_vary); if (_wrapper.getETag()!=null) setHeader("ETag",_wrapper.getETag()); @@ -289,7 +301,7 @@ public abstract class AbstractCompressedStream extends ServletOutputStream { long length=_wrapper.getContentLength(); if (_response.isCommitted() || (length >= 0 && length < _wrapper.getMinCompressSize())) - doNotCompress(); + doNotCompress(false); else if (lengthToWrite > _wrapper.getMinCompressSize()) doCompress(); else @@ -299,7 +311,7 @@ public abstract class AbstractCompressedStream extends ServletOutputStream { long length=_wrapper.getContentLength(); if (_response.isCommitted() || (length >= 0 && length < _wrapper.getMinCompressSize())) - doNotCompress(); + doNotCompress(false); else if (lengthToWrite >= (_bOut.getBuf().length - _bOut.getCount())) doCompress(); } diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/gzip/CompressedResponseWrapper.java b/jetty-http/src/main/java/org/eclipse/jetty/http/gzip/CompressedResponseWrapper.java index 5be638eab47..0ddf8199871 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/gzip/CompressedResponseWrapper.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/gzip/CompressedResponseWrapper.java @@ -138,8 +138,6 @@ public abstract class CompressedResponseWrapper extends HttpServletResponseWrapp (_mimeTypes==null && ct!=null && ct.contains("gzip") || _mimeTypes!=null && (ct==null||!_mimeTypes.contains(StringUtil.asciiToLowerCase(ct))))) { - // Remove the vary header, because of content type. - setHeader("Vary",null); noCompression(); } } @@ -318,7 +316,7 @@ public abstract class CompressedResponseWrapper extends HttpServletResponseWrapp { try { - _compressedStream.doNotCompress(); + _compressedStream.doNotCompress(false); } catch (IOException e) { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GzipHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GzipHandler.java index 51d82db7e4d..68075f49e57 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GzipHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/handler/GzipHandler.java @@ -68,6 +68,7 @@ public class GzipHandler extends HandlerWrapper protected Set _excluded; protected int _bufferSize = 8192; protected int _minGzipSize = 256; + protected String _vary = "Accept-Encoding, User-Agent"; /* ------------------------------------------------------------ */ /** @@ -161,6 +162,31 @@ public class GzipHandler extends HandlerWrapper } } + /* ------------------------------------------------------------ */ + /** + * @return The value of the Vary header set if a response can be compressed. + */ + public String getVary() + { + return _vary; + } + + /* ------------------------------------------------------------ */ + /** + * Set the value of the Vary header sent with responses that could be compressed. + *

+ * By default it is set to 'Accept-Encoding, User-Agent' since IE6 is excluded by + * default from the excludedAgents. If user-agents are not to be excluded, then + * this can be set to 'Accept-Encoding'. Note also that shared caches may cache + * many copies of a resource that is varied by User-Agent - one per variation of the + * User-Agent, unless the cache does some normalization of the UA string. + * @param vary The value of the Vary header set if a response can be compressed. + */ + public void setVary(String vary) + { + _vary = vary; + } + /* ------------------------------------------------------------ */ /** * Get the buffer size. @@ -297,7 +323,7 @@ public class GzipHandler extends HandlerWrapper @Override protected AbstractCompressedStream newCompressedStream(HttpServletRequest request,HttpServletResponse response) throws IOException { - return new AbstractCompressedStream("gzip",request,this) + return new AbstractCompressedStream("gzip",request,this,_vary) { @Override protected DeflaterOutputStream createStream() throws IOException diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/GzipFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/GzipFilter.java index 36ecb1fd352..e49e9f1ee22 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/GzipFilter.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/GzipFilter.java @@ -104,15 +104,21 @@ import org.eclipse.jetty.util.log.Logger; * instead. * * excludePathPatterns Same as excludePath, but accepts regex patterns for more complex matching. + * + * vary Set to the value of the Vary header sent with responses that could be compressed. By default it is + * set to 'Vary: Accept-Encoding, User-Agent' since IE6 is excluded by default from the excludedAgents. + * If user-agents are not to be excluded, then this can be set to 'Vary: Accept-Encoding'. Note also + * that shared caches may cache copies of a resource that is varied by User-Agent - one per variation of + * the User-Agent, unless the cache does some normalization of the UA string. * */ public class GzipFilter extends UserAgentFilter { private static final Logger LOG = Log.getLogger(GzipFilter.class); public final static String GZIP="gzip"; - public final static String ETAG_GZIP="-gzip\""; + public final static String ETAG_GZIP="--gzip\""; public final static String DEFLATE="deflate"; - public final static String ETAG_DEFLATE="-deflate\""; + public final static String ETAG_DEFLATE="--deflate\""; public final static String ETAG="o.e.j.s.GzipFilter.ETag"; protected ServletContext _context; @@ -125,6 +131,7 @@ public class GzipFilter extends UserAgentFilter protected Set _excludedAgentPatterns; protected Set _excludedPaths; protected Set _excludedPathPatterns; + protected String _vary="Accept-Encoding, User-Agent"; private static final int STATE_SEPARATOR = 0; private static final int STATE_Q = 1; @@ -202,6 +209,10 @@ public class GzipFilter extends UserAgentFilter while (tok.hasMoreTokens()) _excludedPathPatterns.add(Pattern.compile(tok.nextToken())); } + + tmp=filterConfig.getInitParameter("vary"); + if (tmp!=null) + _vary=tmp; } /* ------------------------------------------------------------ */ @@ -224,9 +235,9 @@ public class GzipFilter extends UserAgentFilter HttpServletRequest request=(HttpServletRequest)req; HttpServletResponse response=(HttpServletResponse)res; - // Exclude URIs - no Vary because no matter what client, this URI is always excluded + // If not a GET or an Excluded URI - no Vary because no matter what client, this URI is always excluded String requestURI = request.getRequestURI(); - if (isExcludedPath(requestURI)) + if (!HttpMethods.GET.equalsIgnoreCase(request.getMethod()) || isExcludedPath(requestURI)) { super.doFilter(request,response,chain); return; @@ -244,54 +255,45 @@ public class GzipFilter extends UserAgentFilter return; } } - - // Inform caches that responses may vary according to Accept-Encoding (this may be nulled later) - response.setHeader("Vary","Accept-Encoding"); - // Exclude User-Agents + // Excluded User-Agents String ua = getUserAgent(request); - String compressionType = selectCompression(request.getHeader("accept-encoding")); + boolean ua_excluded=ua!=null&&isExcludedAgent(ua); - // If this request is not excluded by agent and if it can be compressed - if (!isExcludedAgent(ua) && compressionType!=null && !response.containsHeader("Content-Encoding") && !HttpMethods.HEAD.equalsIgnoreCase(request.getMethod())) + // Acceptable compression type + String compressionType = ua_excluded?null:selectCompression(request.getHeader("accept-encoding")); + + // Special handling for etags + String etag = request.getHeader("If-None-Match"); + if (etag!=null) { - // Special handling for etags - String etag = request.getHeader("If-None-Match"); - if (etag!=null) - { - if (etag.endsWith(ETAG_GZIP)) - request.setAttribute(ETAG,etag.substring(0,etag.length()-ETAG_GZIP.length())+'"'); - else if (etag.endsWith(ETAG_DEFLATE)) - request.setAttribute(ETAG,etag.substring(0,etag.length()-ETAG_DEFLATE.length())+'"'); - } - - CompressedResponseWrapper wrappedResponse = createWrappedResponse(request,response,compressionType); - - boolean exceptional=true; - try - { - super.doFilter(request,wrappedResponse,chain); - exceptional=false; - } - finally - { - Continuation continuation = ContinuationSupport.getContinuation(request); - if (continuation.isSuspended() && continuation.isResponseWrapped()) - { - continuation.addContinuationListener(new ContinuationListenerWaitingForWrappedResponseToFinish(wrappedResponse)); - } - else if (exceptional && !response.isCommitted()) - { - wrappedResponse.resetBuffer(); - wrappedResponse.noCompression(); - } - else - wrappedResponse.finish(); - } + int dd=etag.indexOf("--"); + if (dd>0) + request.setAttribute(ETAG,etag.substring(0,dd)+(etag.endsWith("\"")?"\"":"")); } - else + + CompressedResponseWrapper wrappedResponse = createWrappedResponse(request,response,compressionType); + + boolean exceptional=true; + try { - super.doFilter(request,new VaryResponseWrapper(response),chain); + super.doFilter(request,wrappedResponse,chain); + exceptional=false; + } + finally + { + Continuation continuation = ContinuationSupport.getContinuation(request); + if (continuation.isSuspended() && continuation.isResponseWrapped()) + { + continuation.addContinuationListener(new ContinuationListenerWaitingForWrappedResponseToFinish(wrappedResponse)); + } + else if (exceptional && !response.isCommitted()) + { + wrappedResponse.resetBuffer(); + wrappedResponse.noCompression(); + } + else + wrappedResponse.finish(); } } @@ -387,14 +389,32 @@ public class GzipFilter extends UserAgentFilter protected CompressedResponseWrapper createWrappedResponse(HttpServletRequest request, HttpServletResponse response, final String compressionType) { CompressedResponseWrapper wrappedResponse = null; - if (compressionType.equals(GZIP)) + if (compressionType==null) { wrappedResponse = new CompressedResponseWrapper(request,response) { @Override protected AbstractCompressedStream newCompressedStream(HttpServletRequest request,HttpServletResponse response) throws IOException { - return new AbstractCompressedStream(compressionType,request,this) + return new AbstractCompressedStream(null,request,this,_vary) + { + @Override + protected DeflaterOutputStream createStream() throws IOException + { + return null; + } + }; + } + }; + } + else if (compressionType.equals(GZIP)) + { + wrappedResponse = new CompressedResponseWrapper(request,response) + { + @Override + protected AbstractCompressedStream newCompressedStream(HttpServletRequest request,HttpServletResponse response) throws IOException + { + return new AbstractCompressedStream(compressionType,request,this,_vary) { @Override protected DeflaterOutputStream createStream() throws IOException @@ -412,7 +432,7 @@ public class GzipFilter extends UserAgentFilter @Override protected AbstractCompressedStream newCompressedStream(HttpServletRequest request,HttpServletResponse response) throws IOException { - return new AbstractCompressedStream(compressionType,request,this) + return new AbstractCompressedStream(compressionType,request,this,_vary) { @Override protected DeflaterOutputStream createStream() throws IOException @@ -422,7 +442,7 @@ public class GzipFilter extends UserAgentFilter }; } }; - } + } else { throw new IllegalStateException(compressionType + " not supported"); @@ -572,11 +592,8 @@ public class GzipFilter extends UserAgentFilter ct=ct.substring(0,colon); } - if (_mimeTypes!=null && !_mimeTypes.contains(StringUtil.asciiToLowerCase(ct))) - // Remove the vary header, because of content type. - super.setHeader("Vary",null); - else - super.setHeader("Vary","Accept-Encoding"); + if (_mimeTypes!=null && _mimeTypes.contains(StringUtil.asciiToLowerCase(ct))) + super.setHeader("Vary",_vary); } } diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/IncludableGzipFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/IncludableGzipFilter.java index 8ff4608a707..5ba2be0022f 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/IncludableGzipFilter.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/IncludableGzipFilter.java @@ -70,14 +70,41 @@ public class IncludableGzipFilter extends GzipFilter protected CompressedResponseWrapper createWrappedResponse(HttpServletRequest request, HttpServletResponse response, final String compressionType) { CompressedResponseWrapper wrappedResponse = null; - if (compressionType.equals(GZIP)) + if (compressionType==null) { wrappedResponse = new IncludableResponseWrapper(request,response) { @Override protected AbstractCompressedStream newCompressedStream(HttpServletRequest request,HttpServletResponse response) throws IOException { - return new AbstractCompressedStream(compressionType,request,this) + return new AbstractCompressedStream(null,request,this,_vary) + { + @Override + protected DeflaterOutputStream createStream() throws IOException + { + return null; + } + + @Override + protected void setHeader(String name, String value) + { + super.setHeader(name, value); + HttpServletResponse response = (HttpServletResponse)getResponse(); + if (!response.containsHeader(name)) + response.setHeader("org.eclipse.jetty.server.include." + name, value); + } + }; + } + }; + } + else if (compressionType.equals(GZIP)) + { + wrappedResponse = new IncludableResponseWrapper(request,response) + { + @Override + protected AbstractCompressedStream newCompressedStream(HttpServletRequest request,HttpServletResponse response) throws IOException + { + return new AbstractCompressedStream(compressionType,request,this,_vary) { @Override protected DeflaterOutputStream createStream() throws IOException @@ -104,7 +131,7 @@ public class IncludableGzipFilter extends GzipFilter @Override protected AbstractCompressedStream newCompressedStream(HttpServletRequest request,HttpServletResponse response) throws IOException { - return new AbstractCompressedStream(compressionType,request,this) + return new AbstractCompressedStream(compressionType,request,this,_vary) { @Override protected DeflaterOutputStream createStream() throws IOException diff --git a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/gzip/GzipTester.java b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/gzip/GzipTester.java index 7146a34e677..7cac0d3afd8 100644 --- a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/gzip/GzipTester.java +++ b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/gzip/GzipTester.java @@ -306,13 +306,13 @@ public class GzipTester { Assert.assertThat("Response.method",response.getMethod(),nullValue()); Assert.assertThat("Response.status",response.getStatus(),is(status)); + Assert.assertThat("Response.header[Content-Encoding]",response.getHeader("Content-Encoding"),not(containsString(compressionType))); if (expectedFilesize != (-1)) { Assert.assertThat("Response.header[Content-Length]",response.getHeader("Content-Length"),notNullValue()); int serverLength = Integer.parseInt(response.getHeader("Content-Length")); Assert.assertThat("Response.header[Content-Length]",serverLength,is(expectedFilesize)); } - Assert.assertThat("Response.header[Content-Encoding]",response.getHeader("Content-Encoding"),not(containsString(compressionType))); } private HttpTester executeRequest(String uri) throws IOException, Exception @@ -465,6 +465,7 @@ public class GzipTester ServletHolder servletHolder = servletTester.addServlet(servletClass,"/"); servletHolder.setInitParameter("baseDir",testdir.getDir().getAbsolutePath()); FilterHolder holder = servletTester.addFilter(gzipFilterClass,"/*",0); + holder.setInitParameter("vary","Accept-Encoding"); return holder; }