[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[shukitchan]

Github user shukitchan closed the pull request at:

https://github.com/apache/trafficserver/pull/1516


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[shukitchan]

Github user shukitchan commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r106500402
  
--- Diff: doc/admin-guide/plugins/combo_handler.en.rst ---
@@ -83,3 +83,16 @@ results in these file paths being "reconstructed"::
 /dir:path2/file5
 /dir:path2/file6
 
+Caching
+===
+Combohandler follows a few rules for the "Cache-Control" header:
+
+1) All requested documents must have "immutable" for the combo'd
--- End diff --

yeah, you are right. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[danobi]

Github user danobi commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r106499461
  
--- Diff: doc/admin-guide/plugins/combo_handler.en.rst ---
@@ -83,3 +83,16 @@ results in these file paths being "reconstructed"::
 /dir:path2/file5
 /dir:path2/file6
 
+Caching
+===
+Combohandler follows a few rules for the "Cache-Control" header:
+
+1) All requested documents must have "immutable" for the combo'd
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[shukitchan]

Github user shukitchan commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r106484837
  
--- Diff: doc/admin-guide/plugins/combo_handler.en.rst ---
@@ -83,3 +83,16 @@ results in these file paths being "reconstructed"::
 /dir:path2/file5
 /dir:path2/file6
 
+Caching
+===
+Combohandler follows a few rules for the "Cache-Control" header:
+
+1) All requested documents must have "immutable" for the combo'd
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[danobi]

Github user danobi commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r104771989
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -187,6 +207,94 @@ InterceptData::~InterceptData()
   }
 }
 
+void
+CacheControlHeader::update(TSMBuffer bufp, TSMLoc hdr_loc)
+{
+  vector values;
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[danobi]

Github user danobi commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r104584519
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -187,6 +207,94 @@ InterceptData::~InterceptData()
   }
 }
 
+void
+CacheControlHeader::update(TSMBuffer bufp, TSMLoc hdr_loc)
+{
+  vector values;
+  bool found_immutable = false;
+  bool found_private   = false;
+
+  // Load each value from the Cache-Control header into the vector values
+  TSMLoc field_loc = TSMimeHdrFieldFind(bufp, hdr_loc, 
TS_MIME_FIELD_CACHE_CONTROL, TS_MIME_LEN_CACHE_CONTROL);
+  if (field_loc != TS_NULL_MLOC) {
+int n_values = TSMimeHdrFieldValuesCount(bufp, hdr_loc, field_loc);
+if ((n_values != TS_ERROR) && (n_values > 0)) {
+  for (int i = 0; i < n_values; i++) {
+// Grab this current header value
+int _val_len = 0;
+const char *_val = TSMimeHdrFieldValueStringGet(bufp, hdr_loc, 
field_loc, i, &_val_len);
+string val(_val, _val_len);
+
+// We want the header value to be lowercase since CC headers are 
case insensitive
+transform(val.begin(), val.end(), val.begin(), ::tolower);
+values.push_back(val);
+  }
+} else {
+  TSHandleMLocRelease(bufp, hdr_loc, field_loc);
+  return;
+}
+TSHandleMLocRelease(bufp, hdr_loc, field_loc);
+  } else {
+return;
+  }
+
+  for (auto const  : values) {
+// Update max-age if necessary
+if (val.find(TS_HTTP_VALUE_MAX_AGE) != string::npos) {
+  int max_age = -1;
+  char *ptr   = const_cast(val.c_str());
+  ptr += TS_HTTP_LEN_MAX_AGE;
+  while ((*ptr == ' ') || (*ptr == '\t'))
+ptr++;
+  if (*ptr == '=') {
+ptr++;
+max_age = atoi(ptr);
+  }
+  if (max_age > 0 && max_age < _max_age) {
+_max_age = max_age;
+  }
+  // If we find even a single occurrence of private, the whole 
response must be private
+} else if (val.find(TS_HTTP_VALUE_PRIVATE) != string::npos) {
+  found_private = true;
+  // Every requested document must have immutable for the final 
response to be immutable
+} else if (val.find(HTTP_IMMUTABLE) != string::npos) {
+  found_immutable = true;
+}
+  }
+
+  if (!found_immutable) {
+LOG_DEBUG("Did not see an immutable cache control. The response will 
be not be immutable");
+_immutable = false;
+  }
+
+  if (found_private) {
+LOG_DEBUG("Saw a private cache control. The response will be private");
+_public  = false;
+_private = true;
+  }
+}
+
+string
+CacheControlHeader::generate() const
+{
+  char line_buf[256];
+  const char *publicity;
+  const char *immutable;
+
+  if (_public) {
+publicity = TS_HTTP_VALUE_PUBLIC;
+  } else if (_private) {
+publicity = TS_HTTP_VALUE_PRIVATE;
+  } else {
+// Default is public
+publicity = TS_HTTP_VALUE_PUBLIC;
+  }
+  immutable = (_immutable ? ", " HTTP_IMMUTABLE : "");
+
+  sprintf(line_buf, "Cache-Control: max-age=%d, %s%s\r\n", _max_age, 
publicity, immutable);
--- End diff --

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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[danobi]

Github user danobi commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r103987142
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -136,6 +138,24 @@ struct InterceptData {
   ~InterceptData();
 };
 
+/*
+ * This class is responsible for keeping track of and processing the 
various
+ * Cache-Control values between all the requested documents
+ */
+struct CacheControlHeader {
+  // Update the object with a document's Cache-Control header
+  void update(TSMBuffer bufp, TSMLoc hdr_loc);
+
+  // Return the Cache-Control for the combined document
+  string generate() const;
+
+  // Cache-Control values we're keeping track of
+  int _max_age= 31536; // max value (10 years)
+  bool _public= true;
+  bool _private   = false;
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[danobi]

Github user danobi commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r103965565
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -136,6 +138,24 @@ struct InterceptData {
   ~InterceptData();
 };
 
+/*
+ * This class is responsible for keeping track of and processing the 
various
+ * Cache-Control values between all the requested documents
+ */
+struct CacheControlHeader {
+  // Update the object with a document's Cache-Control header
+  void update(TSMBuffer bufp, TSMLoc hdr_loc);
+
+  // Return the Cache-Control for the combined document
+  string generate() const;
+
+  // Cache-Control values we're keeping track of
+  int _max_age= 31536; // max value (10 years)
--- End diff --

Actually I believe the max value should be 1 year. According to [RFC 
2616](https://www.ietf.org/rfc/rfc2616.txt), 
```
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.
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[danobi]

Github user danobi commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r103965747
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -136,6 +138,24 @@ struct InterceptData {
   ~InterceptData();
 };
 
+/*
+ * This class is responsible for keeping track of and processing the 
various
+ * Cache-Control values between all the requested documents
+ */
+struct CacheControlHeader {
+  // Update the object with a document's Cache-Control header
+  void update(TSMBuffer bufp, TSMLoc hdr_loc);
+
+  // Return the Cache-Control for the combined document
+  string generate() const;
+
+  // Cache-Control values we're keeping track of
+  int _max_age= 31536; // max value (10 years)
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r103611267
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -187,6 +207,94 @@ InterceptData::~InterceptData()
   }
 }
 
+void
+CacheControlHeader::update(TSMBuffer bufp, TSMLoc hdr_loc)
+{
+  vector values;
--- End diff --

Perhaps convolute the search logic. In that case there would be a 
`std::array` 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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r103610662
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -136,6 +138,24 @@ struct InterceptData {
   ~InterceptData();
 };
 
+/*
+ * This class is responsible for keeping track of and processing the 
various
+ * Cache-Control values between all the requested documents
+ */
+struct CacheControlHeader {
+  // Update the object with a document's Cache-Control header
+  void update(TSMBuffer bufp, TSMLoc hdr_loc);
+
+  // Return the Cache-Control for the combined document
+  string generate() const;
+
+  // Cache-Control values we're keeping track of
+  int _max_age= 31536; // max value (10 years)
+  bool _public= true;
+  bool _private   = false;
--- End diff --

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[shukitchan]

Github user shukitchan commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r103590528
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -187,6 +207,94 @@ InterceptData::~InterceptData()
   }
 }
 
+void
+CacheControlHeader::update(TSMBuffer bufp, TSMLoc hdr_loc)
+{
+  vector values;
+  bool found_immutable = false;
+  bool found_private   = false;
+
+  // Load each value from the Cache-Control header into the vector values
+  TSMLoc field_loc = TSMimeHdrFieldFind(bufp, hdr_loc, 
TS_MIME_FIELD_CACHE_CONTROL, TS_MIME_LEN_CACHE_CONTROL);
+  if (field_loc != TS_NULL_MLOC) {
+int n_values = TSMimeHdrFieldValuesCount(bufp, hdr_loc, field_loc);
+if ((n_values != TS_ERROR) && (n_values > 0)) {
+  for (int i = 0; i < n_values; i++) {
+// Grab this current header value
+int _val_len = 0;
+const char *_val = TSMimeHdrFieldValueStringGet(bufp, hdr_loc, 
field_loc, i, &_val_len);
+string val(_val, _val_len);
+
+// We want the header value to be lowercase since CC headers are 
case insensitive
+transform(val.begin(), val.end(), val.begin(), ::tolower);
+values.push_back(val);
+  }
+} else {
+  TSHandleMLocRelease(bufp, hdr_loc, field_loc);
+  return;
+}
+TSHandleMLocRelease(bufp, hdr_loc, field_loc);
+  } else {
+return;
+  }
+
+  for (auto const  : values) {
+// Update max-age if necessary
+if (val.find(TS_HTTP_VALUE_MAX_AGE) != string::npos) {
+  int max_age = -1;
+  char *ptr   = const_cast(val.c_str());
+  ptr += TS_HTTP_LEN_MAX_AGE;
+  while ((*ptr == ' ') || (*ptr == '\t'))
+ptr++;
+  if (*ptr == '=') {
+ptr++;
+max_age = atoi(ptr);
+  }
+  if (max_age > 0 && max_age < _max_age) {
+_max_age = max_age;
+  }
+  // If we find even a single occurrence of private, the whole 
response must be private
+} else if (val.find(TS_HTTP_VALUE_PRIVATE) != string::npos) {
+  found_private = true;
+  // Every requested document must have immutable for the final 
response to be immutable
+} else if (val.find(HTTP_IMMUTABLE) != string::npos) {
+  found_immutable = true;
+}
+  }
+
+  if (!found_immutable) {
+LOG_DEBUG("Did not see an immutable cache control. The response will 
be not be immutable");
+_immutable = false;
+  }
+
+  if (found_private) {
+LOG_DEBUG("Saw a private cache control. The response will be private");
+_public  = false;
+_private = true;
+  }
+}
+
+string
+CacheControlHeader::generate() const
+{
+  char line_buf[256];
+  const char *publicity;
+  const char *immutable;
+
+  if (_public) {
+publicity = TS_HTTP_VALUE_PUBLIC;
+  } else if (_private) {
+publicity = TS_HTTP_VALUE_PRIVATE;
+  } else {
+// Default is public
+publicity = TS_HTTP_VALUE_PUBLIC;
+  }
+  immutable = (_immutable ? ", " HTTP_IMMUTABLE : "");
+
+  sprintf(line_buf, "Cache-Control: max-age=%d, %s%s\r\n", _max_age, 
publicity, immutable);
--- End diff --

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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[shukitchan]

Github user shukitchan commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r103587579
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -136,6 +138,24 @@ struct InterceptData {
   ~InterceptData();
 };
 
+/*
+ * This class is responsible for keeping track of and processing the 
various
+ * Cache-Control values between all the requested documents
+ */
+struct CacheControlHeader {
+  // Update the object with a document's Cache-Control header
+  void update(TSMBuffer bufp, TSMLoc hdr_loc);
+
+  // Return the Cache-Control for the combined document
+  string generate() const;
+
+  // Cache-Control values we're keeping track of
+  int _max_age= 31536; // max value (10 years)
+  bool _public= true;
+  bool _private   = false;
--- End diff --

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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[shukitchan]

Github user shukitchan commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1516#discussion_r103586416
  
--- Diff: plugins/esi/combo_handler.cc ---
@@ -136,6 +138,24 @@ struct InterceptData {
   ~InterceptData();
 };
 
+/*
+ * This class is responsible for keeping track of and processing the 
various
+ * Cache-Control values between all the requested documents
+ */
+struct CacheControlHeader {
+  // Update the object with a document's Cache-Control header
+  void update(TSMBuffer bufp, TSMLoc hdr_loc);
+
+  // Return the Cache-Control for the combined document
+  string generate() const;
+
+  // Cache-Control values we're keeping track of
+  int _max_age= 31536; // max value (10 years)
--- End diff --

probably should be unsigned?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1516: Implement Cache-Control: immutable handlin...

[danobi]

GitHub user danobi opened a pull request:

https://github.com/apache/trafficserver/pull/1516

Implement Cache-Control: immutable handling

This patch makes `combo_handler` correctly handle the presence of one or 
more 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.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/danobi/trafficserver combocache_immutable

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/trafficserver/pull/1516.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1516


commit 3dfd7ef0da3915e257caafd2fea92d4812dbeb21
Author: Daniel Xu 
Date:   2017-02-24T15:36:50Z

Implement Cache-Control: immutable for combohandler

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.

commit 9f9d6340dc3772ca13fb2d0ad2e8ed420c256019
Author: Daniel Xu 
Date:   2017-02-28T21:18:02Z

Handle private header & code cleanup

Now we handle Cache-Control: private directives. If any of the responses
has a private directive, then the combo response should also be private.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---