Re: [PATCH] Error page format codes upgrade

On 06/12/2013 02:13 AM, Amos Jeffries wrote:

> I'm not quite sure exactly, but IFF it is possible for async operations
> about request #1 in a pipeline to complete after a response to that
> request has already been sent, it pops the context off teh queue and may
> result in a second response (ie error page) being generated with details
> pulled from context of request #2 in the pipeline.

I believe the following scenario is the most likely one (among the
broken cases): The error page for request#2 is being generated while the
response for request#1 is still being served. In that case, error#2 will
use request#1 ALE.

>> My question was about incorrect ClientSocketContext object being used
>> for error page generation, regardless of the accessor that led to that
>> object. So, after this change, if pipelining is enabled, then generated
>> error pages may contain completely bogus information, right? Was that
>> true before the change? Or is this a problem with the current error page
>> generation code as well?
>
> There are a few comments in places using getCurrentContext() to the
> effect that it sometimes supplies the wrong context in pipelines.

getCurrentContext() or currentobject corresponds to the earliest
unanswered request. That may not be the request that we are forming an
error page for. I think there are no serious doubts about that
possibility because processing multiple requests concurrently is what
pipelining is about,and that may cause concurrent responses.

The real question IMO is whether the current trunk code relies on
getCurrentContext() or currentobject to form an error page. AFACT, it
does not (but please correct me if I am wrong). The proposed patch does.
That change worries me because pipelining setups may start serving bogus
error pages if the patch is committed. There are several options to
resolve this:

A1) Prove that the trunk code already uses request#1 ALE for the error
page#2 generation. In other words, trunk is already broken, and your
patch is not making it worse.

A2) Prove that the proposed code can never use request#1 ALE for the
error page#2 generation. In other words, your patch is correct when it
comes to pipelining ALEs.

B) Supply ALE to the error page generation code without using
getCurrentContext()/currentobject, to avoid pipelining problems.

C) Do not use getCurrentContext()/currentobject [when pipelining is
enabled], together with the ALE object they provide, to make the patch
no worse than the current code. This will result in some sacrifices in
functionality [when pipelining is enabled], but may still produce
correct code worth committing.

HTH,

Alex.
Received on Wed Jun 12 2013 - 17:07:08 MDT