On Mar 24, 2014, at 9:45 AM, Leif Hedstrom <[email protected]> wrote:
> Hi all,
>
>
> 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%.
Nice!
> 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”);
I don't like how this mixes memory handling in the same API. I think that the
consistent form of this API should be:
TSHttpTxnErrorBodySet(txnp, body, body_len, content_type,
content_type_len);
I agree that a string literal is probably the most common usage, but we should
make it behave the same as other APIs.
> 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).
I'd be pretty surprised if anyone was taking advantage of the body factory
stuff. One suggestion is to add a flags field (with valid values checking). For
example,
TSHttpTxnErrorBodySet(txnp, 0, body, -1, content_type, -1);
TSHttpTxnErrorBodySet(txnp, TS_BODY_INTERPOLATE_LOG_TAGS, body, -1,
content_type, -1);
> 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).
>
> Finally, I’ve attached the current patch to the Jira:
Which Jira was that?
J
