Re: [AUDIT PATCH] comm cleanup - pconn changes

On 05/10/10 13:47, Amos Jeffries wrote:
> On Mon, 04 Oct 2010 14:31:25 -0600, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
>> On 10/02/2010 10:55 AM, Amos Jeffries wrote:
>>> Implements the pconn and IdleConnList as storage lists of
>>> Comm::Connection.
>>>
>>> With Comm::Connection the key definition has to change. The old keying
>>> based solely on domain would produce a Connection object whose FD did
>>> not exactly match the remote IP:port selected by the new routing
> logics.
>>>
>>> I've kept the concept that the pconn key is the details to describe the
>>> remote end of the link used to fetch any given domain. As such its
>>> become much simpler now. Just the remote IP:port from the TCP details
>>> and requested domain (post re-writing) or peer hostname for peer pconn.
>>>
>>> The change also removes the client address from relevance (enhancement
>>> bug fix). Instead we scan the list of pconn going to our desired
>>> remote-end for the local-end details (tcp_outgoing_addr or tproxy IPs)
>>> to pick an exact pconn match (bug fix).
>>
>> .
>>
>>> - list->removeFD(fd); /* might delete list */
>>> - comm_close(fd);
>>> + /* might delete list */
>>> + if (list&& list->remove(conn)) {
>>> + Comm::ConnectionPointer nonConst = conn;
>>> + nonConst->close();
>>> + }
>>
>> The list member cannot be NULL until the remove is called. The nonConst
>> conversion is also weird but I will discuss that later. Should probably
>> be rewritten as:
>>
>> Comm::ConnectionPointer deletedConn =
>> list->remove(conn); // might delete list
>> if (deletedConn != NULL)
>> deletedConn->close();
>>
>> However, this is still not equivalent to the old code because the old
>> code was closing the connection regardless of whether it was found in
>> the list. Is that an intentional change? Perhaps add a comment about it?

After a closer look. PconnPool::pop is the only external user of
remove() and that is already doing fingUseable().

By doing:
  * move that instance of remove() into the end of findUseable().
  * move findIndex() out of remove() into the Read and Timeout methods.
  * make remove(conn) into removeAt(int) taking the found index,

results in:
  * making findUseable equivalent to a search + pop()
  * removing one pass over the list in the 'fast' path.

>>
>>> - list->removeFD(fd); /* might delete list */
>>> - comm_close(fd);
>>> + Comm::ConnectionPointer temp = new Comm::Connection; // XXX:
>>> transition. make timeouts pass conn in
>>> + temp->fd = fd;
>>> + if (list->remove(temp)) {
>>> + temp->close();
>>> + } else
>>> + temp->fd = -1; // XXX: transition. prevent temp erasure
>>> double-closing FD until timeout CB passess conn in.
>>
>> Lost that critical, IMO, "might delete list" comment.

Fixed.

>>
>> Also, the creation of a temporary connection seems unnecessary when we
>> have a findIndex method. Use findIndex or, better, add a findConn method

findIndex() takes a conn called temp and locates it's index...

Named it better now: findIndexOf(temp)

>
>> if you want to close the found connection.
>>
>> The same concerns about not-closing of the not-found connection apply
>> here as well.
>>
>>
>>> + for (int index = 0; index< nfds; index++)
>>> + theList[index] = oldList[index];
>>
>> This makes IdleConnList::push slower that the old code that used
>> xmemcpy(). The shifting overhead is one of the reasons we should not use
>> [C] arrays here, IMO.

Um, I didn't think it safe to memcpy an array of virtual classes.
If it is we should do it in removeAt() as well for faster shuffle there
too where its more critical.

push() is not a critical path, since it occurs between requests.

>>
>>
>>> -IdleConnList::read(int fd, char *buf, size_t len, comm_err_t flag, int
>>> xerrno, void *data)
>>> +IdleConnList::read(const Comm::ConnectionPointer&conn, char *buf,
>>> size_t len, comm_err_t flag, int xerrno, void *data)
>>
>> Please capitalize static Read() if you are changing its profile anyway.
>>

Done.

>>
>>> + int used = strlen(buf);
>>
>> Please use const.

Done.

>>> + Comm::ConnectionPointer nonConst = conn;
>>> + Comm::ConnectionPointer nonConst = conn;
>>> + Comm::ConnectionPointer nonConst = conn;
>> ...
>>
>> This needs to be fixed somehow as it is only going to get worse. There
>> are two questions here:

Fixed. The Boost shared_ptr seems to be the better model to follow as
it is a true counting ptr type where auto_ptr is not.

>>
>>> + /**
>>> + * Updates destLink to point at an existing open connection if
>>> available and retriable.
>>> + * Otherwise, return false.
>>> + *
>>> + * We close available persistent connection if the caller
>>> transaction is not
>>> + * retriable to avoid having a growing number of open connections
>>> when many
>>> + * transactions create persistent connections but are not
> retriable.
>>> + */
>>> + bool pop(Comm::ConnectionPointer&serverConn, const char *domain,
>>> bool retriable);
>>
>> Why not always return the [possibly nil] popped connection instead?
>> Seems like a simpler and safer interface then separating the serverConn
>> value from the pop() status value.
>
> Hmm, will think it over. It started that way, I can't recall the details
> why I changed it.

Found it, saved some lines in the callers. On second thoughts, doing pop
the other way while being a bit more code lets the forwarding keep its
'key' serverDestinations[0] conn unchanged for a second lookup if the
first fails.

Reverted this change.

>>
>> Consider:
>> * Manages idle persistent connections to a caller-defined set of
>> * servers (e.g., all HTTP servers). Uses a collection of IdleConnLists
>> * internally. Controls lists existence and limits the total number of
>> * idle connections across the collection.
>>

Done.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.8
   Beta testers wanted for 3.2.0.2