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

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

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

 Amos
Received on Wed Apr 06 2011 - 23:50:11 MDT