You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2014/08/04 19:22:01 UTC
svn commit: r1615697 - in /tomcat/trunk:
java/org/apache/catalina/servlets/DefaultServlet.java
webapps/docs/changelog.xml
Author: markt
Date: Mon Aug 4 17:22:01 2014
New Revision: 1615697
URL: http://svn.apache.org/r1615697
Log:
When the gzip option is enabled for the DefaultServlet ensure that a suitable Vary header is returned for resources that might be returned directly in compressed form.
Modified:
tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java?rev=1615697&r1=1615696&r2=1615697&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java (original)
+++ tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java Mon Aug 4 17:22:01 2014
@@ -799,16 +799,15 @@ public class DefaultServlet extends Http
// Serve a gzipped version of the file if present
boolean usingGzippedVersion = false;
- if (gzip &&
- resource.isFile() &&
- !included &&
- !path.endsWith(".gz") &&
- checkIfGzip(request)) {
+ if (gzip && resource.isFile() && !path.endsWith(".gz")) {
WebResource gzipResource = resources.getResource(path + ".gz");
if (gzipResource.exists() && gzipResource.isFile()) {
- response.addHeader("Content-Encoding", "gzip");
- resource = gzipResource;
- usingGzippedVersion = true;
+ response.addHeader("Vary", "accept-encoding");
+ if (!included && checkIfGzip(request)) {
+ response.addHeader("Content-Encoding", "gzip");
+ resource = gzipResource;
+ usingGzippedVersion = true;
+ }
}
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1615697&r1=1615696&r2=1615697&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Mon Aug 4 17:22:01 2014
@@ -80,6 +80,12 @@
and it need not be fatal when the Realm starts. Based on a patch by
Cédric Couralet. (markt)
</fix>
+ <fix>
+ When the <code>gzip</code> option is enabled for the
+ <code>DefaultServlet</code> ensure that a suitable <code>Vary</code>
+ header is returned for resources that might be returned directly in
+ compressed form.
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1615697 - in /tomcat/trunk: java/org/apache/catalina/servlets/DefaultServlet.java
webapps/docs/changelog.xml
Posted by Konstantin Kolinko <kn...@gmail.com>.
2014-08-12 14:19 GMT+04:00 Mark Thomas <ma...@apache.org>:
> On 12/08/2014 00:14, Konstantin Kolinko wrote:
>> 2014-08-04 21:22 GMT+04:00 <ma...@apache.org>:
>>> Author: markt
>>> Date: Mon Aug 4 17:22:01 2014
>>> New Revision: 1615697
>>>
>>> URL: http://svn.apache.org/r1615697
>>> Log:
>>> When the gzip option is enabled for the DefaultServlet ensure that a suitable Vary header is returned for resources that might be returned directly in compressed form.
>>>
>>> Modified:
>>> tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java
>>> tomcat/trunk/webapps/docs/changelog.xml
>>>
>>> Modified: tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java
>>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java?rev=1615697&r1=1615696&r2=1615697&view=diff
>>> ==============================================================================
>>> --- tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java (original)
>>> +++ tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java Mon Aug 4 17:22:01 2014
>>> @@ -799,16 +799,15 @@ public class DefaultServlet extends Http
>>>
>>> // Serve a gzipped version of the file if present
>>> boolean usingGzippedVersion = false;
>>> - if (gzip &&
>>> - resource.isFile() &&
>>> - !included &&
>>
>> 1). I do get why "!included" check was moved to the place below. I
>> think it should not be moved.
>>
>> I think that "included" flag does not depend on "accept-encoding"
>> value sent by user.
>
> Agreed. Fixed.
Good.
>> 2). There may already be a Vary header in the response.
>
> Agreed. A filter could set one. This has been fixed too.
The updated HTTP specification does not allow multiple Vary headers in
the response.
Multiple headers are allowed only if value is defined as a list, but
here it is defined as "asterisk or a list".
>> 3) The code may check response.isCommitted(). (In other words, that
>> addHeader("Content-Encoding", "gzip")
>> call has been successful).
>
> Is that necessary? It isn't something we check anywhere else in the
> DefaultServlet (and I am not aware of any problems in that area).
Just an additional check to add to the condition where "if(!included)"
is checked.
I guess that majority of such use cases are already covered by
"if(!included)" check.
It matters only if some content have already been generated by a Filter.
This gz file serving feature is relatively new and it is off by
default, so I think there are not many who are using it to report
errors.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1615697 - in /tomcat/trunk: java/org/apache/catalina/servlets/DefaultServlet.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 12/08/2014 00:14, Konstantin Kolinko wrote:
> 2014-08-04 21:22 GMT+04:00 <ma...@apache.org>:
>> Author: markt
>> Date: Mon Aug 4 17:22:01 2014
>> New Revision: 1615697
>>
>> URL: http://svn.apache.org/r1615697
>> Log:
>> When the gzip option is enabled for the DefaultServlet ensure that a suitable Vary header is returned for resources that might be returned directly in compressed form.
>>
>> Modified:
>> tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java
>> tomcat/trunk/webapps/docs/changelog.xml
>>
>> Modified: tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java
>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java?rev=1615697&r1=1615696&r2=1615697&view=diff
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java (original)
>> +++ tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java Mon Aug 4 17:22:01 2014
>> @@ -799,16 +799,15 @@ public class DefaultServlet extends Http
>>
>> // Serve a gzipped version of the file if present
>> boolean usingGzippedVersion = false;
>> - if (gzip &&
>> - resource.isFile() &&
>> - !included &&
>
> 1). I do get why "!included" check was moved to the place below. I
> think it should not be moved.
>
> I think that "included" flag does not depend on "accept-encoding"
> value sent by user.
Agreed. Fixed.
> 2). There may already be a Vary header in the response.
Agreed. A filter could set one. This has been fixed too.
> 3) The code may check response.isCommitted(). (In other words, that
> addHeader("Content-Encoding", "gzip")
> call has been successful).
Is that necessary? It isn't something we check anywhere else in the
DefaultServlet (and I am not aware of any problems in that area).
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1615697 - in /tomcat/trunk: java/org/apache/catalina/servlets/DefaultServlet.java
webapps/docs/changelog.xml
Posted by Konstantin Kolinko <kn...@gmail.com>.
2014-08-04 21:22 GMT+04:00 <ma...@apache.org>:
> Author: markt
> Date: Mon Aug 4 17:22:01 2014
> New Revision: 1615697
>
> URL: http://svn.apache.org/r1615697
> Log:
> When the gzip option is enabled for the DefaultServlet ensure that a suitable Vary header is returned for resources that might be returned directly in compressed form.
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java
> tomcat/trunk/webapps/docs/changelog.xml
>
> Modified: tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java?rev=1615697&r1=1615696&r2=1615697&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java (original)
> +++ tomcat/trunk/java/org/apache/catalina/servlets/DefaultServlet.java Mon Aug 4 17:22:01 2014
> @@ -799,16 +799,15 @@ public class DefaultServlet extends Http
>
> // Serve a gzipped version of the file if present
> boolean usingGzippedVersion = false;
> - if (gzip &&
> - resource.isFile() &&
> - !included &&
1). I do get why "!included" check was moved to the place below. I
think it should not be moved.
I think that "included" flag does not depend on "accept-encoding"
value sent by user.
2). There may already be a Vary header in the response.
RFC 7230 Chapter 3.2.2. Field Order
[quote]
A sender MUST NOT generate multiple header fields with the same field
name in a message unless either the entire field value for that
header field is defined as a comma-separated list [i.e., #(values)]
or the header field is a well-known exception (as noted below).
[/quote]
The well-known exception referenced above is "Set-Cookie" header.
RFC 7231 Chapter 7.1.4. Vary
defines the field as
Vary = "*" / 1#field-name
so it is an alternative between "*" versus a list of values, so the
above requirement for allowing multiple headers is not met.
3) The code may check response.isCommitted(). (In other words, that
addHeader("Content-Encoding", "gzip")
call has been successful).
Best regards,
Konstantin Kolinko
> - !path.endsWith(".gz") &&
> - checkIfGzip(request)) {
> + if (gzip && resource.isFile() && !path.endsWith(".gz")) {
> WebResource gzipResource = resources.getResource(path + ".gz");
> if (gzipResource.exists() && gzipResource.isFile()) {
> - response.addHeader("Content-Encoding", "gzip");
> - resource = gzipResource;
> - usingGzippedVersion = true;
> + response.addHeader("Vary", "accept-encoding");
> + if (!included && checkIfGzip(request)) {
> + response.addHeader("Content-Encoding", "gzip");
> + resource = gzipResource;
> + usingGzippedVersion = true;
> + }
> }
> }
>
>
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1615697&r1=1615696&r2=1615697&view=diff
> ==============================================================================
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Mon Aug 4 17:22:01 2014
> @@ -80,6 +80,12 @@
> and it need not be fatal when the Realm starts. Based on a patch by
> Cédric Couralet. (markt)
> </fix>
> + <fix>
> + When the <code>gzip</code> option is enabled for the
> + <code>DefaultServlet</code> ensure that a suitable <code>Vary</code>
> + header is returned for resources that might be returned directly in
> + compressed form.
> + </fix>
> </changelog>
> </subsection>
> <subsection name="Coyote">
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org