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

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
              errors: false false
             tunnels: false true
------------------

Please review as a mid-term fix candidate.

Thank you,

Alex.