On 04/06/2013 11:48 PM, Amos Jeffries wrote:
> What this does is convert ErrorState object to using the generic
> libformat.la parser and macro expansions instead of its own rather
> limited custom ones for error pages and deny_info URL creation.
This is a very nice improvement IMO, thank you.
> class AccessLogEntry: public RefCountable
> {
...
> +
> + /// details of the error page presented to the client (if any)
> + ErrorState *errorDetails;
> };
Please do not use a general name like "errorDetails" to store
information dedicated to a very specific use by internal ErrorPage code.
Besides, we already have a whole class called ErrorDetail and it has
nothing to do with the above member. I suggest calling this new member
"errorPage".
Please also add a comment that explains that this pointer is managed
internally by ErrorState and should not be used outside ErrorState code.
You have documented that in your email, but I think it is worth
documenting that in the code. More on the lifetime of this pointer below.
> + MemBuf *res = ConvertText(m);
> +
> + // yuck! steal the res buffer for our output
> + result.buf = res->buf;
> + result.size = res->size;
> + result.capacity = res->capacity;
> + result.max_capacity = res->max_capacity;
> + res->stolen = true; // prevent delete erasing the buffer we stole.
> + delete res;
Please rewrite without stealing. One way to do that would be to pass
MemBuf pointer to ConvertText. If the passed pointer is NULL,
ConvertText() will create a new buffer.
> + // XXX: if request exists, we might use ALE from request->lientConnectionManager->http.al ??
It sounds like a great idea because error page ALE is missing
information that the http.al has. On the other hand, if we set
http.al.errorDetails to our page, it is not obvious that another error
page for the same transaction will never do that as well, overwriting
our pointer and confusing things. I wonder if we can do better:
As far as I can tell, the useful lifetime of the new errorDetails
pointer is the fmt.assemble() call. If that is true, let's document that
fact and rewrite the code to set and clear the pointer just before and
just after that call. You may even assert that the ale_->errorDetails
pointer is nil in the error page destructor.
If we can do the above, then we should use http.al when it is available.
Refcounting should make that easy.
> *b) Doing a whole parse cycle for every error response is very costly.
> We are already loading the text page files on every error response. This
> adds the un-optimized libformat parser lag on top of that.
Have you tested the performance impact of these changes? Perhaps they
are not as bad as they sound because the old code was not super
optimized either?
> *c) For now AccessLogEntry equired by the libformat API is generated by
> ErrorState just before use. Ideally the code creating the ErrorState
> should pass it an already filled out ALE object for the transaction.
Unfortunately, a lot of code creating error pages is in no better
position than ErrorState to find that valuable ALE object. This is the
old problem of a missing "master transaction" object: HttpRequest is
available nearly everywhere but is declared (probably correctly!)
unsuitable for logging in favor of ALE which is not easily available in
many contexts.
I suspect that the only reasonable mid-term solution is to carefully add
a pointer to the "master transaction" ALE to HttpRequest.
> - Ip::Address src_addr;
> + Ip::Address src_addr; // client Comm::Connection ??
Please see if you can delete this member. You may have removed the only
useful code that was using it. The dumping code is not very useful and
can can probably be rewritten to get this info from elsewhere (since the
ALE formatting code does support logging of the client source address
and does not have access to this member).
> - case '%':
> - p = "%";
> - break;
> -
> default:
> - mb.Printf("%%%c", token);
> - do_quote = 0;
> break;
> }
After your changes, if an old error page had %x text, where "x" was not
a supported error page macro name, is it possible that the finalized
error page (as seen by users) may change? If so, this should be
documented in the change log, I think.
> + // parse the text using Format:: parser
> + Format::Format fmt("errorPageTemporary");
> + fmt.parse(text);
If possible, please create fmt later, when it is actually needed for the
assemble() call.
Thank you,
Alex.
Received on Thu Apr 18 2013 - 17:42:01 MDT
This archive was generated by hypermail 2.2.0 : Fri Apr 19 2013 - 12:00:12 MDT