Re: [PATCH] Error page format codes upgrade

On 13/06/2013 5:06 a.m., Alex Rousskov wrote:
> 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.

Arg. Yes, exactly so.

>
>>> 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.

Going to have to go with B+C for now.

Amos
Received on Thu Jun 13 2013 - 03:13:16 MDT