Re: fixing deferred reads

Alex Rousskov wrote:
> On Fri, 2008-09-26 at 13:35 +1200, Amos Jeffries wrote:
>>> On Thu, 2008-09-25 at 13:43 -0600, Alex Rousskov wrote:
>>>> I can probably fix it myself, but it would help a lot if somebody
>>>> could
>>>> document (briefly!) the overall purpose of deferred reads and the
>>>> exact
>>>> purposes of these two nearly identical methods (one brief description
>>>> for each, please):
>>> From memory...
>>>
>>>> DeferredReadManager::kickReads(int const count)
>>> do up to count reads that have been deferred
>>>
>>>
>>>> and
>>>>
>>>> DeferredReadManager::flushReads()
>>> do all reads
>>>
>> Which makes more sense then to cleanup make flushReads() == kickReads(-1),
>> or similar instead of duplication.
>
> Avoiding duplication was not "possible" when the class was written
> because of the reentrant callbacks and lack of size() accounting in the
> cbdata list used by the class. The author was, apparently, worried that
> the reads list will grow as it is being emptied, causing an infinite
> loop.

I think I remember the kickReads() commit in a fuzzy way. I think it was
a quick hack to prevent CPU overload and delays on high-performance
sites when the list was large. Done without full knowlege of the
deferred reads code, then ported from 2.6 without fixing.

>
> I think we can merge the two methods now because callbacks can no longer
> be reentrant. I will add a \todo.

I wasn't criticizing the original design, merely making a point about
that is now may be easily fixable. The TODO is good if there is no time
or code-monkey minion to do a patch.

Amos

-- 
Please use Squid 2.7.STABLE4 or 3.0.STABLE9