Re: client_side and comm_close

Hi all,
  I think we have two different problems here, the client_side code and
the comm_close in async calls.
If I am not wrong the bug which triggered this discussion is the bug2309
which is squid3.0 bug where AsyncCalls does not exists. The problem here
is the client side code which the true is that it needs clean up. It is
well known to everyone tried to fix bugs or add code in this part of
squid3..

About the comm_close, the AsyncCalls designed and implemented in such
way to require the minimum changes in the squid3 code and this was
limiting some thinks...

Henrik Nordstrom wrote:
>
> Here is a different proposal. Split async calls in two. Queued and
> immediate, with the event loops using immediate calls and everything
> else queued. The siplest way of doing so safely would be to make async
> calls immediate if not already within an async call, but adjusting the
> event loops is also a possibility. This would make the code behave a lot
> more like how it was before async, eleminating most races.
>
>
> The very few cases which may depend on comm_close being immediate should
> be possible to identify by cbdataInternalLock calls I think as the state
> object previously may disappear immediately otherwise. I can't see any
> in the client_side_* code today. But there very likely is assumptions
> that after comm_close has been called no further events will fire
> positively on that fd (only as ERR_COMM_CLOSING).
>
> I don't remember why comm_close was made async, but the more I think

I think (but not sure now) there was cases where some of the pending
asyncCalls must executed.

> about it I think it's most likely wrong. comm_close SHOULD invalidate
> any pending comm operations on that fd, anything else just leads to
> nightmares.

To invalidate any pending comm operation for a particular fd you should
scan all AsyncCalls queue which maybe is costly in a busy proxy, with
thousands of asyncCalls scheduled (maybe the use of secondary asyncCalls
indexes for each fd?)....
If we have only AsyncJob based asyncCalls it would be easy to not
execute (invalidate) AsyncCalls which are referred to non valid
asyncJobs, but now still exists functions which are scheduled using the
old way...

>
> Related to this I also think the COMM_ERR_CLOSING callback from comm
> should be dropped. Nearly all handlers looking for COMM_ERR_CLOSING do
> nothing else than return immediately to abort execution.. and the few
> exceptions there is can be dealt with by a close callback if at all
> needed:
>
> errorSendComplete, invalidates the err object
>
> ftpStateData::dataRead, sets a boolean that the read is done
>
> HttpStateData::handleRequestBodyProducerAborted injects a
> COMM_ERR_CLOSING
>
> HttpStateData::readReply sets a boolean that the read is done
>
> ServerStateData::sentRequestBody clears requestSender
>
>
> the other 20 or so users of comm callbacks just returns immediately on
> COMM_ERR_CLOSING, "aborting" the call..
>
> Regards
> Henrik
>
>
Received on Tue Apr 22 2008 - 14:40:23 MDT