[TS-2657] Eliminate TSHttpTxnSetHttpRetBody() and improve upon TSHttpTxnErrorBodySet()

• Type: Improvement
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 5.0.0
• Component/s: TS API
• Labels: api-change
• Assignee: Leif Hedstrom
• Reporter: Leif Hedstrom
• Created: 24/Mar/14 16:43
• Updated: 18/Dec/14 05:12
• Resolved: 02/Apr/14 22:55

Description

We currently have two APIs to set an “error” body. One was added at Yahoo many years ago, and it was done for the wrong reasons (premature optimization being one of them). I’ve made a patch that unifies these two APIs into one, such that you can use TSHttpTxnErrorBodySet() anywhere you could use either of those other two APIs. There are some subtle changes in how the APIs work, which I’ll describe here.

1. First, both TSHttpTxnSetHttpRetBody() and TSHttpTxnGetMaxHttpRetBodySize() are removed. I’ve patched all “core” plugins to use the TSHttpTxnErrorBodySet() API, but any non-Apache plugin would break. Hence, this change is going in for v5.0.0, which allows breaking compatibility.

2. More importantly, TSHttpTxnErrorBodySet() requires for the body to be heap allocated, whereas TSHttpTxnSetHttpRetBody() used a fixed size (2KB) array on the HttpSM. This was the primary reason why I started looking into this, since this “simple” change reduces the size of the HttpSM by 25%.

3. In order to make the TSHttpTxnErrorBodySet() API easier to use, I’ve changed the content type argument to not transfer ownership. The common (well, exclusive) pattern using this API used to be

TSHttpTxnErrorBodySet(txnp, body, body_len, TSstrdup(“plain/html”));

This just seems like an overly generalized case, for a problem that doesn’t exist (clearly we can all manage some static strings . The new API is simply:

TSHttpTxnErrorBodySet(txnp, body, body_len, “plain/html”);

4. The TSHttpTxnErrorBodySet() also imposes the body to be sent through the body factory, for log field expansion. For starters, this is incredibly inefficient , as well as imposing an 8KB size limit. This is something I intend to ameliorate soon by improvements to the body factory. However, this is an unnecessary inefficiency IMO; As part of the body factory improvements, I also plan on exposing the body factory “log field” expansion as a generic API. This would allow any plugin to decide if they want to go through the pains of body factory expansions or not (and not limited to just error pages, e.g. Header Rewrite would use this new API).

So, I’d like to open this up for discussions and concerns. I think 3) and 4) are the primary candidates for concerns and objections. I’m willing to compromise either or both if there are major concerns (for example, is it worth the “cost” of changing the API in 3) ? I think it is, but I can easily be persuaded otherwise).

Issue Links

• breaks: TS-2886 Fix regression where body factory is not used