Implement Cache-Control: immutable handling #1516

danobi · 2017-02-28T21:28:24Z

This patch makes combo_handler correctly handle the presence of one or more Cache-Control: immutable flags.

In short, the combo response will be immutable if and only if all requested documents have immutable in their response headers.

In addition, the same logic applies for Cache-Control: private. This was not present before.

atsci · 2017-02-28T21:30:13Z

Linux build failed! See https://ci.trafficserver.apache.org/job/linux-github/1541/ for details.

danobi · 2017-02-28T21:31:48Z

I'm on the fence about including the Cache-Control: private changes in this patch. On one hand, it makes sense for that to be in this refactor. On the other, it has nothing to do with immutable. Maybe change the commit name?

atsci · 2017-02-28T21:41:31Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1647/ for details.

atsci · 2017-02-28T21:43:19Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/79/ for details.

atsci · 2017-02-28T21:46:14Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1543/ for details.

shukitchan · 2017-02-28T21:47:46Z

I would like to do the review on this.

atsci · 2017-02-28T21:50:49Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1649/ for details.

atsci · 2017-02-28T21:53:30Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/81/ for details.

atsci · 2017-02-28T22:05:34Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1546/ for details.

atsci · 2017-02-28T22:08:20Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/211/ for details.

atsci · 2017-02-28T22:34:14Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/213/ for details.

shukitchan · 2017-03-01T00:32:52Z

plugins/esi/combo_handler.cc

+  string generate() const;
+
+  // Cache-Control values we're keeping track of
+  int _max_age    = 315360000; // max value (10 years)


probably should be unsigned?

Or ssize_t. Is the 10 year value from the HTTP spec?

Actually I believe the max value should be 1 year. According to RFC 2616,

To mark a response as "never expires," an origin server sends an Expires date approximately one year from the time the response is sent. HTTP/1.1 servers SHOULD NOT send Expires dates more than one year in the future.

I'll remove a 0 from the constant if everyone agrees.

shukitchan · 2017-03-01T00:42:10Z

plugins/esi/combo_handler.cc

+  // Cache-Control values we're keeping track of
+  int _max_age    = 315360000; // max value (10 years)
+  bool _public    = true;
+  bool _private   = false;


I find it rather confusing to maintain two booleans here - _public and _private.
Is there any other kind of value?
If not, perhaps we can simply this to be just one boolean?

Hmmm, perhaps a tri-value enum? private, public, default?

Yeah the tri-enum sounds better. I knew that public & private were disjoint, but there was the default case I wanted to handle too.

shukitchan · 2017-03-01T00:51:39Z

I am not in favor with the "private" related changes because i think it can catch some existing users by surprise. And we did not mention anything in the document as well - https://docs.trafficserver.apache.org/en/latest/admin-guide/plugins/combo_handler.en.html

Speaking of the document, it would be great if we can update that, too. Thanks.

shukitchan · 2017-03-01T00:55:34Z

The max-age related code is also a bit different from before. Previously we can have max-age larger than 315360000. Now with this change, I think we can no longer have max-age larger than that. right?

shukitchan · 2017-03-01T00:58:57Z

plugins/esi/combo_handler.cc

+      TSHandleMLocRelease(bufp, hdr_loc, field_loc);
+      return;
+    }
+    TSHandleMLocRelease(bufp, hdr_loc, field_loc);


This piece seems a bit redundant? from line 232 to 236, i mean

shukitchan · 2017-03-01T01:00:21Z

will try to compile it tonight and see if i have more comments.

shukitchan · 2017-03-01T01:05:19Z

plugins/esi/combo_handler.cc

+  }
+  immutable = (_immutable ? ", " HTTP_IMMUTABLE : "");
+
+  sprintf(line_buf, "Cache-Control: max-age=%d, %s%s\r\n", _max_age, publicity, immutable);


if we happen to support and have a case of having a really large (!) number for max age, we might have buffer overflow.
Then we might need to consider snprintf or something safer?

The longest possible value is -(2^64) which has some finite number of digits. That should be computable and line_buf adjusted to suffice in that case.

2^64 ~ 1.84x10^{19}, so it won't be over 20 digits long. The buffer is 256 chars wide so I think we're good.

shukitchan · 2017-03-01T07:00:27Z

the patch compiles fine. And it seems to run through clang-format as well already. thanks. pls check out the comments above.

SolidWallOfCode · 2017-03-01T04:58:40Z

plugins/esi/combo_handler.cc

+  string generate() const;
+
+  // Cache-Control values we're keeping track of
+  int _max_age    = 315360000; // max value (10 years)


Or ssize_t. Is the 10 year value from the HTTP spec?

SolidWallOfCode · 2017-03-01T05:03:36Z

plugins/esi/combo_handler.cc

+void
+CacheControlHeader::update(TSMBuffer bufp, TSMLoc hdr_loc)
+{
+  vector<string> values;


Perhaps convolute the search logic. In that case there would be a std::array<char const*, 3> initialized to the 3 values of interest. Each item in the loop at 221 would have an inner loop like that starting at 241. Then there's no need to allocate a vector as a temporary, and the comparisons can be done using strcasecmp to avoid the copy as well.

This would also simplify the logic Kit was concerned about at 232-236.

Why a loop? If we use a loop then it's harder to set the found_private and found_immutable flags. I feel like unrolling the loop in this case is better since the # of parameters in Cache-Control is known.

SolidWallOfCode · 2017-03-01T14:21:42Z

I agree with the private/public refactor. The code would be significantly uglier if processing for the cache control field was split.

atsci · 2017-03-07T04:13:18Z

FreeBSD build successful! See https://ci.trafficserver.apache.org/job/freebsd-github/1685/ for details.

atsci · 2017-03-07T04:16:26Z

Intel CC build successful! See https://ci.trafficserver.apache.org/job/icc-github/117/ for details.

atsci · 2017-03-07T04:21:00Z

Linux build successful! See https://ci.trafficserver.apache.org/job/linux-github/1581/ for details.

atsci · 2017-03-07T04:32:12Z

clang-analyzer build successful! See https://ci.trafficserver.apache.org/job/clang-analyzer-github/249/ for details.

atsci · 2017-03-16T15:38:33Z

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/331/

atsci · 2017-03-16T15:42:47Z

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/332/

shukitchan · 2017-03-16T17:03:21Z

looks good. thanks.
can you update this as well? https://github.com/apache/trafficserver/blob/master/doc/admin-guide/plugins/combo_handler.en.rst

@SolidWallOfCode , any other comments you have?

danobi · 2017-03-16T17:08:22Z

I didn't realize there was a doc page for that. Updating now.

danobi · 2017-03-16T17:29:33Z

I just copied over the same text from the README to the admin config docs.

atsci · 2017-03-16T17:31:17Z

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/76/

atsci · 2017-03-16T17:31:29Z

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/89/

atsci · 2017-03-16T17:33:34Z

AU check successful! https://ci.trafficserver.apache.org/job/autest-github/73/

atsci · 2017-03-16T17:38:44Z

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/1771/

atsci · 2017-03-16T17:41:10Z

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1664/

atsci · 2017-03-16T17:41:32Z

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/202/

atsci · 2017-03-16T17:58:24Z

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/334/

shukitchan · 2017-03-16T17:42:35Z

doc/admin-guide/plugins/combo_handler.en.rst

+=======
+Combohandler follows a few rules for the "Cache-Control" header:
+
+1) All requested documents must have "immutable" for the combo'd


just nitpicking. I think for markdown, the syntax of "ordered list" is just "1. "
right?

Not that I knew this beforehand, but this is restructured text and it looks like this is the correct list syntax. Got lucky I guess.

https://en.wikipedia.org/wiki/ReStructuredText#Lists

yeah, you are right.

shukitchan · 2017-03-17T09:38:21Z

can you squash now? then i can verify and merge.

Before, combohandler would not insert the immutable cache control even if all the requested documents had the header. Now, combohandler will respect the presence of the header.

danobi · 2017-03-17T13:24:16Z

done

atsci · 2017-03-17T13:26:11Z

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/80/

atsci · 2017-03-17T13:26:40Z

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/93/

atsci · 2017-03-17T13:28:23Z

AU check successful! https://ci.trafficserver.apache.org/job/autest-github/77/

atsci · 2017-03-17T13:33:33Z

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/1775/

atsci · 2017-03-17T13:35:46Z

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1668/

atsci · 2017-03-17T13:36:41Z

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/206/

atsci · 2017-03-17T13:53:05Z

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/338/

shukitchan added the Plugins label Feb 28, 2017

shukitchan self-requested a review February 28, 2017 21:46

shukitchan added this to the 7.2.0 milestone Feb 28, 2017

shukitchan self-assigned this Feb 28, 2017

shukitchan requested a review from SolidWallOfCode February 28, 2017 21:48

shukitchan reviewed Mar 1, 2017

View reviewed changes

SolidWallOfCode reviewed Mar 1, 2017

View reviewed changes

shukitchan reviewed Mar 16, 2017

View reviewed changes

shukitchan approved these changes Mar 17, 2017

View reviewed changes

Implement Cache-Control: immutable for combohandler

a73e93a

Before, combohandler would not insert the immutable cache control even if all the requested documents had the header. Now, combohandler will respect the presence of the header.

danobi force-pushed the combocache_immutable branch from 0dff5de to a73e93a Compare March 17, 2017 13:23

shukitchan merged commit 9d33ca2 into apache:master Mar 20, 2017

zwoop modified the milestones: 7.2.0, 8.0.0 Apr 25, 2017

Implement Cache-Control: immutable handling #1516

Implement Cache-Control: immutable handling #1516

Conversation

danobi commented Feb 28, 2017 • edited

atsci commented Feb 28, 2017

danobi commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

shukitchan commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

atsci commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shukitchan commented Mar 1, 2017

shukitchan commented Mar 1, 2017

Choose a reason for hiding this comment

shukitchan commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shukitchan commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SolidWallOfCode commented Mar 1, 2017

atsci commented Mar 7, 2017

atsci commented Mar 7, 2017

atsci commented Mar 7, 2017

atsci commented Mar 7, 2017

atsci commented Mar 16, 2017

atsci commented Mar 16, 2017

shukitchan commented Mar 16, 2017

danobi commented Mar 16, 2017

danobi commented Mar 16, 2017

atsci commented Mar 16, 2017

atsci commented Mar 16, 2017

atsci commented Mar 16, 2017

atsci commented Mar 16, 2017

atsci commented Mar 16, 2017

atsci commented Mar 16, 2017

atsci commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shukitchan commented Mar 17, 2017

danobi commented Mar 17, 2017

atsci commented Mar 17, 2017

atsci commented Mar 17, 2017

atsci commented Mar 17, 2017

atsci commented Mar 17, 2017

atsci commented Mar 17, 2017

atsci commented Mar 17, 2017

atsci commented Mar 17, 2017

danobi commented Feb 28, 2017 •

edited