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.
