On 05/10/10 13:47, Amos Jeffries wrote:
On Mon, 04 Oct 2010 14:31:25 -0600, Alex Rousskov
<[email protected]> 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
