You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "hnakamur (via GitHub)" <gi...@apache.org> on 2023/06/18 02:43:29 UTC
[GitHub] [trafficserver] hnakamur opened a new pull request, #9877: Do not add content-length for status 204 cache
hnakamur opened a new pull request, #9877:
URL: https://github.com/apache/trafficserver/pull/9877
In master and version 9.2.1, content-length header is added when serving the cache with status 204 No Content.
It seems caused by [Make 204 cacheable again by maskit · Pull Request #9333 · apache/trafficserver](https://github.com/apache/trafficserver/pull/9333).
With this pull request, content-length is not added when serving the cache with status 204 No Content.
I made test cases at https://github.com/hnakamur/atstest-go.
* The test log *with this fix* is at [log/atsmasterfix-test.log](https://github.com/hnakamur/atstest-go/blob/4c2f9228d05ecfe37486f023503910f6be7e5892/log/atsmasterfix-test.log) and there is no content-length in the status 204 response from the cache: [log/atsmasterfix-test.log#L35-L41](https://github.com/hnakamur/atstest-go/blob/4c2f9228d05ecfe37486f023503910f6be7e5892/log/atsmasterfix-test.log#L35-L41).
* The test log *without this fix* is at [log/atsmaster-test.log](https://github.com/hnakamur/atstest-go/blob/4c2f9228d05ecfe37486f023503910f6be7e5892/log/atsmaster-test.log) and there is content-length in the status 204 response from the cache: [log/atsmaster-test.log#L40](https://github.com/hnakamur/atstest-go/blob/4c2f9228d05ecfe37486f023503910f6be7e5892/log/atsmaster-test.log#L40).
(Note aside: the response headers in above logs are sorted alphabetically so the order is not same as the received response).
I also made the same fix for trafficserver version 9.2.1 at https://github.com/hnakamur/trafficserver/tree/9_2_1_dont_add_content_length_for_status_204_cache
* The test log with this fix is at [log/ats921fix-test.log](https://github.com/hnakamur/atstest-go/blob/4c2f9228d05ecfe37486f023503910f6be7e5892/log/ats921fix-test.log) and there is no content-length in the status 204 response from the cache: [log/ats921fix-test.log#L35-L41](https://github.com/hnakamur/atstest-go/blob/4c2f9228d05ecfe37486f023503910f6be7e5892/log/ats921fix-test.log#L35-L41).
* The test log without this fix is at [log/ats921-test.log](https://github.com/hnakamur/atstest-go/blob/4c2f9228d05ecfe37486f023503910f6be7e5892/log/ats921-test.log) and there is content-length in the status 204 response from the cache: [log/ats921-test.log#L40](https://github.com/hnakamur/atstest-go/blob/4c2f9228d05ecfe37486f023503910f6be7e5892/log/ats921-test.log#L40).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #9877: Do not add content-length for status 204 cache
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #9877:
URL: https://github.com/apache/trafficserver/pull/9877#issuecomment-1607974883
Cherry picked to the 9.2.x branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9877: Do not add content-length for status 204 cache
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9877:
URL: https://github.com/apache/trafficserver/pull/9877#issuecomment-1598025061
[approve ci rocky]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9877: Do not add content-length for status 204 cache
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9877:
URL: https://github.com/apache/trafficserver/pull/9877#discussion_r1235794525
##########
proxy/http/HttpTransact.cc:
##########
@@ -6748,7 +6748,9 @@ HttpTransact::handle_content_length_header(State *s, HTTPHdr *header, HTTPHdr *b
change_response_header_because_of_range_request(s, header);
s->hdr_info.trust_response_cl = true;
} else {
- header->set_content_length(cl);
+ if (!(s->source == SOURCE_CACHE && header->status_get() == HTTP_STATUS_NO_CONTENT)) {
Review Comment:
Can you tell me why `s->source == SOURCE_CACHE` is needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit merged pull request #9877: Do not add content-length for status 204 cache
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit merged PR #9877:
URL: https://github.com/apache/trafficserver/pull/9877
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] hnakamur commented on a diff in pull request #9877: Do not add content-length for status 204 cache
Posted by "hnakamur (via GitHub)" <gi...@apache.org>.
hnakamur commented on code in PR #9877:
URL: https://github.com/apache/trafficserver/pull/9877#discussion_r1236001947
##########
proxy/http/HttpTransact.cc:
##########
@@ -6748,7 +6748,9 @@ HttpTransact::handle_content_length_header(State *s, HTTPHdr *header, HTTPHdr *b
change_response_header_because_of_range_request(s, header);
s->hdr_info.trust_response_cl = true;
} else {
- header->set_content_length(cl);
+ if (!(s->source == SOURCE_CACHE && header->status_get() == HTTP_STATUS_NO_CONTENT)) {
Review Comment:
I think in general the reverse proxy is supposed to send the reponse from the origin server as is, so I thought it would be better to limit the guard not to add content-length to the case for sending from the status 204 cache.
However, as I'm writing this, I noticed that it would also delete content-length when sending the status 204 cache after receiving a status 204 response having content-length from an origin server. So the code above is modifying the response from an origin server after all.
So, should we avoid to add content-length for both the case when it sends from cache or when it proxies the response from an origin server? If so, is `if (header->status_get() != HTTP_STATUS_NO_CONTENT)` the correct?
I ran my tests for `if (header->status_get() != HTTP_STATUS_NO_CONTENT)` and confirmed they pass.
https://github.com/hnakamur/atstest-go/commit/aaeccc240c80d78167e3b2b52d0781f2fb05cdc8
https://github.com/hnakamur/trafficserver/commit/2faa3f9ab72b0b6dbfa02ebb831b98cb8340d033
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] hnakamur commented on a diff in pull request #9877: Do not add content-length for status 204 cache
Posted by "hnakamur (via GitHub)" <gi...@apache.org>.
hnakamur commented on code in PR #9877:
URL: https://github.com/apache/trafficserver/pull/9877#discussion_r1237692951
##########
proxy/http/HttpTransact.cc:
##########
@@ -6748,7 +6748,9 @@ HttpTransact::handle_content_length_header(State *s, HTTPHdr *header, HTTPHdr *b
change_response_header_because_of_range_request(s, header);
s->hdr_info.trust_response_cl = true;
} else {
- header->set_content_length(cl);
+ if (!(s->source == SOURCE_CACHE && header->status_get() == HTTP_STATUS_NO_CONTENT)) {
Review Comment:
Thanks for your explanation. I pushed https://github.com/apache/trafficserver/pull/9877/commits/2faa3f9ab72b0b6dbfa02ebb831b98cb8340d033.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9877: Do not add content-length for status 204 cache
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9877:
URL: https://github.com/apache/trafficserver/pull/9877#discussion_r1237359022
##########
proxy/http/HttpTransact.cc:
##########
@@ -6748,7 +6748,9 @@ HttpTransact::handle_content_length_header(State *s, HTTPHdr *header, HTTPHdr *b
change_response_header_because_of_range_request(s, header);
s->hdr_info.trust_response_cl = true;
} else {
- header->set_content_length(cl);
+ if (!(s->source == SOURCE_CACHE && header->status_get() == HTTP_STATUS_NO_CONTENT)) {
Review Comment:
Having different behavior for the two sources (origin server and cache) doesn't make sense to me. The document in cache is originally from an origin server and it's just cached. If we removed the header only when we serve a document from cache, a client would see inconsistent responses that depend on the cache status. So, yes, I think we should check only the status code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org