Re: [PATCH] 3.2.0.6 assertion failed: comm.cc:216: "fd_table[fd].halfClosedReader != NULL"

On 04/06/2011 05:50 PM, Amos Jeffries wrote:
> On Wed, 06 Apr 2011 14:01:52 -0600, Alex Rousskov wrote:
>> On 04/06/2011 11:47 AM, Alex Rousskov wrote:
>>> On 04/05/2011 05:38 AM, Amos Jeffries wrote:
>>>> Debugging on IRC revealed this to be caused by some as yet unidentified
>>>> bug in client-side CONNECT handling.
>>>>
>>>> The client-side leaves a read handler registered when tunnel is handed
>>>> the client FD to start pushing bytes. The attached patch works around
>>>> that by making tunnel detect the existing handler and cancel it.
>>
>>> Client side has:
>>>
>>> - do_next_read (several incarnations): usually used to trigger
>>> comm_read in the caller;
>>>
>>> - mayUseConnection: is sometimes used to avoid triggering comm_read
>>> even though its intent was to reserve the connection for the current
>>> request (i.e., the intent was to avoid switching to the next request,
>>> rather than avoiding reading from the socket);
>>>
>>> - conn->flags.readMoreRequests: Kind of like do_next_read but more
>>> persistent as it is stored in the context flags rather than a local
>>> variable. A true do_next_read value can cause the readMoreRequests flag
>>> to be set, but setting do_next_read to false may not clear the flag.
>>>
>>>
>>> I will try to polish the above a little instead of just contributing to
>>> the current insanity, but it may not be possible.
>>
>> The attached patch attempts to clean up the above mess a little and fix
>> the double-read assertion. It passed a few manual tests but needs more
>> testing and possibly more work.
>>
>> --------
>> Polished request reading code to fix CONNECT double-read assertion
>> comm.cc:216: "fd_table[fd].halfClosedReader != NULL"
>>
>> ConnStateData::flags.readMoreRequests, do_next_read variables, and
>> ClientSocketContext::mayUseConnection() methods were used (or unused!)
>> incorrectly or inconsistently.
>>
>> This change removes all do_next_read variables to simplify the state.
>> Instead, the renamed ConnStateData::flags.readMore indicates whether
>> client_side.cc should call comm_read. The mayUseConnection() methods are
>> now used to indicate whether the next client-sent byte (buffered or
>> read) should be reserved for the current request rather than being
>> interpreted as the beginning of the next request.
>>
>> Usually,
>> flags.readMore mayUseConnection
>> regular requests: true false
>> requests with bodies: true true
>
> NOTE this "parse errors" change please.
>
> The 'errors' here do NOT refer to server-side or cached errors, which
> come under "regular requests".

Agreed. I used "malformed requests" since client-side may err for
reasons other than parsing errors. This is just a quick, imprecise
summary anyway, to illustrate that all four combinations are possible.

>> parse errors: false false
>
>> tunnels: false true
>> ------------------
>>
>> Please review as a mid-term fix candidate.
>
> Looks okay to me as a long-term fix. The other changes which will help
> are more in the re-design scope than bug fix.
>
> Thank you fro using this approach. do_next_read parameters were causing
> a bit of trouble for the parser cleanup. Good to know it was useless :)
>
>
> Since this is affecting the core parser routines I would like to see
> this get a as much real-world testing as possible in the short time
> available. Please everyone who can, apply this patch to todays 3.2.0.6
> snapshot bundle and try running it!

Committed to trunk as r11364 per your request on IRC.

HTH,

Alex.
P.S. This is bug #3192 now.
Received on Sat Apr 09 2011 - 04:07:16 MDT