Re: [PATCH] Add DNS lookup step to ICAP connect

On 07/01/2011 02:53 AM, Alex Rousskov wrote:
> On 06/30/2011 11:11 AM, Tsantilas Christos wrote:
>> And this is the third version.
>>
>> This patch does not include only the DNS lookup for ICAP but other
>> changes too like ICAP connections pool changes, which required to allow
>> ICAP client work.
>
> Why do we need to dynamically allocate theIdleConns? Just curious...

Because the idleConnList is a cbdata class I think it should be created
using "new" operator.

Inside the IdleConnList::push method we are creating async calls which
takes "this" pointer as data argument:
  timeoutCall = commCbCall(...,
CommTimeoutCbPtrFun(...,this));

Inside asyncCall, while trying to create a cbdataReference of the
IdleConnList object passed as argument, hit the following assertion
(inside cbdata::check):
    assert(cookie == ((long)this ^ Cookie));

>
>
>> This patch also is not complete. The Max-Connections ICAP feature is
>> currently broken and needs some redesign, to work well.
>
> Sigh. We just got it in!
>
>
>> I think we should move the DNS-lookup and open connection to the ICAP
>> server functionality from Adaptation::Icap::Xaction class to
>> Adaptation::Icap::ServiceRep class.
>
> I am fine with that if you think it is going to make things work again.
> Just be careful when passing the open connection object from Service to
> Xaction. If Xaction is gone, make sure the connection is returned to the
> Service or, at least, closed. IIRC, you already have a custom AsyncCall
> or Dialer that does similar stuff so you can reuse/adjust that code.
>
>
> Thank you,
>
> Alex.
>
>
>> On 06/25/2011 05:04 AM, Amos Jeffries wrote:
>>> Version 2. This one resolves a few compile bugs.
>>>
>>> Ralf: cc'd in case you would like to try testing this.
>>>
>>> On 25/06/11 03:21, Amos Jeffries wrote:
>>>> This seeks to add an async DNS lookup step to the ICAP connection setup
>>>> process.
>>>>
>>>> As a side effect it adds a little bit of tcp_outgoing_address support to
>>>> ICAP. Its limited to "dst" ACL at present. So outgoing ACL selections
>>>> depending on HTTP request details wont work. Which makes sense since
>>>> this connection may be reused for multiple requests.
>>>>
>>>> I've skipped the idea of using more than one IP result since there seems
>>>> to be no sane place to store multiple IPs between connect attempts. It
>>>> currently relies on ipcache MarkGood/MarkBad keeping a good usable IP at
>>>> the top of the IP set.
>>>>
>>>
>>> Amos
>
>
Received on Fri Jul 01 2011 - 08:06:48 MDT