On 02/09/11 18:29, Amos Jeffries wrote:
On 02/09/11 04:55, Alex Rousskov wrote:
On 09/01/2011 05:47 AM, Amos Jeffries wrote:
This patch seeks to avoids race conditions between the AsyncCalls for
the pconn monitoring handlers and lookups pulling pconn up for use.
...
It adds checks for the timeout handler, as discussed in bug 3281,
preventing the related type of race from the timeout handler.
+ // our connection timeout handler is scheduled to run already. unsafe
+ if (fd_table[conn->fd].timeoutHandler == NULL)
+ return false;
I am not strongly against this, but it would be more elegant and may be
more efficient to let the about-to-timeout connections to be reused
instead. This can be safely done, for example, by canceling the pending
timeout notification callback in the connection giving code.
Cancelling calls here would mean re-writing IdleConnList to store those
calls to be cancelled. As it stands right now once scheduled there is no
code outside the AsyncEngine with a pointer to them.
This patch would still be needed after all that change but would cancel
and return true instead of just returning false.
I'll post a new patch shortly. With isAvailable() protecting against
unsafe re-use cases the handlers on both read and timeout can now
unconditionally cleanup and close the idle conn. Read callback may not
even need to bother about the CLOSING flag, but I think the forced
fd_table[] close cycle on shutdown (bug 2110) might still cause that case.
... and here is that.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.15
Beta testers wanted for 3.2.0.10
=== modified file 'src/pconn.cc'
--- src/pconn.cc 2011-07-08 00:12:40 +0000
+++ src/pconn.cc 2011-09-02 05:48:26 +0000
@@ -89,6 +89,7 @@
}
/** Remove the entry at specified index.
+ * May perform a shuffle of list entries to fill the gap.
* \retval false The index is not an in-use entry.
*/
bool
@@ -194,22 +195,34 @@
commSetConnTimeout(conn, Config.Timeout.pconn, timeoutCall);
}
+/// Determine whether an entry in the idle list is available for use.
+/// Returns false if entry is unset, closed, or appears timed out.
+bool
+IdleConnList::isAvailable(int i) const
+{
+ const Comm::ConnectionPointer &conn = theList_[i];
+
+ // connection already closed. useless.
+ if (!Comm::IsConnOpen(conn))
+ return false;
+
+ // our connection timeout handler is scheduled to run already. unsafe
+ if (fd_table[conn->fd].timeoutHandler == NULL)
+ return false;
+
+ // our connection early-read/close handler is scheduled to run already. unsafe
+ if (!COMMIO_FD_READCB(conn->fd)->active())
+ return false;
+
+ return true;
+}
+
Comm::ConnectionPointer
IdleConnList::pop()
{
for (int i=size_-1; i>=0; i--) {
- // Is the FD pending completion of the closure callback?
- // this flag is set while our early-read/close handler is
- // waiting for a remote response. It gets unset when the
- // handler is scheduled.
- //The following check is disabled for now until we have a
- // correct implementation of the read_pending flag
- //if (!fd_table[theList_[i]->fd].flags.read_pending)
- // continue;
-
- // connection already closed. useless.
- if (!Comm::IsConnOpen(theList_[i]))
+ if (!isAvailable(i))
continue;
// finally, a match. pop and return it.
@@ -242,17 +255,7 @@
for (int i=size_-1; i>=0; i--) {
- // Is the FD pending completion of the closure callback?
- // this flag is set while our early-read/close handler is
- // waiting for a remote response. It gets unset when the
- // handler is scheduled.
- //The following check is disabled for now until we have a
- // correct implementation of the read_pending flag
- //if (!fd_table[theList_[i]->fd].flags.read_pending)
- // continue;
-
- // connection already closed. useless.
- if (!Comm::IsConnOpen(theList_[i]))
+ if (!isAvailable(i))
continue;
// local end port is required, but dont match.
@@ -274,27 +277,33 @@
return Comm::ConnectionPointer();
}
+/* might delete list */
+void
+IdleConnList::findAndClose(const Comm::ConnectionPointer &conn)
+{
+ int index = findIndexOf(conn);
+ if (index >= 0) {
+ /* might delete this */
+ removeAt(index);
+ clearHandlers(conn);
+ conn->close();
+ }
+}
+
void
IdleConnList::Read(const Comm::ConnectionPointer &conn, char *buf, size_t len, comm_err_t flag, int xerrno, void *data)
{
debugs(48, 3, HERE << len << " bytes from " << conn);
if (flag == COMM_ERR_CLOSING) {
- /* Bail out early on COMM_ERR_CLOSING - close handlers will tidy up for us */
+ debugs(48, 3, HERE << "COMM_ERR_CLOSING from " << conn);
+ /* Bail out on COMM_ERR_CLOSING - may happen when shutdown aborts our idle FD */
return;
}
IdleConnList *list = (IdleConnList *) data;
- int index = list->findIndexOf(conn);
- if (index >= 0) {
- /* might delete list */
- list->removeAt(index);
- list->clearHandlers(conn);
- }
- // else we lost a race.
- // Somebody started using the pconn since the remote end disconnected.
- // pass the closure info on!
- conn->close();
+ /* may delete list/data */
+ list->findAndClose(conn);
}
void
@@ -302,13 +311,8 @@
{
debugs(48, 3, HERE << io.conn);
IdleConnList *list = static_cast<IdleConnList *>(io.data);
- int index = list->findIndexOf(io.conn);
- assert(index>=0);
- if (index >= 0) {
- /* might delete list */
- list->removeAt(index);
- io.conn->close();
- }
+ /* may delete list/data */
+ list->findAndClose(io.conn);
}
/* ========== PconnPool PRIVATE FUNCTIONS ============================================ */
=== modified file 'src/pconn.h'
--- src/pconn.h 2011-06-17 06:04:05 +0000
+++ src/pconn.h 2011-09-02 05:50:47 +0000
@@ -55,8 +55,10 @@
void closeN(size_t count);
private:
+ bool isAvailable(int i) const;
bool removeAt(int index);
int findIndexOf(const Comm::ConnectionPointer &conn) const;
+ void findAndClose(const Comm::ConnectionPointer &conn);
static IOCB Read;
static CTCB Timeout;
