This patch attempts to implements part 1 of Alex proposed fixes for bug
3565 - crashes with deferred accept().
It replaces the raw-pointers stored in AcceptLimiter with a new
TcpAcceptor::Pointer.
Also adds a bit of TODO documentation about future optimizations which
we might want to consider later.
Amos
=== modified file 'src/comm/AcceptLimiter.cc'
--- src/comm/AcceptLimiter.cc 2012-08-14 11:53:07 +0000
+++ src/comm/AcceptLimiter.cc 2013-03-01 02:32:08 +0000
@@ -7,29 +7,33 @@
Comm::AcceptLimiter Comm::AcceptLimiter::Instance_;
-Comm::AcceptLimiter &Comm::AcceptLimiter::Instance()
+Comm::AcceptLimiter &
+Comm::AcceptLimiter::Instance()
{
return Instance_;
}
void
-Comm::AcceptLimiter::defer(Comm::TcpAcceptor *afd)
+Comm::AcceptLimiter::defer(const Comm::TcpAcceptor::Pointer &afd)
{
- ++ afd->isLimited;
- debugs(5, 5, HERE << afd->conn << " x" << afd->isLimited);
- deferred.push_back(afd);
+ ++ (afd->isLimited);
+ debugs(5, 5, afd->conn << " x" << afd->isLimited);
+ deferred_.push_back(afd);
}
void
-Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor *afd)
+Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor::Pointer &afd)
{
- for (unsigned int i = 0; i < deferred.size() && afd->isLimited > 0; ++i) {
- if (deferred[i] == afd) {
- -- deferred[i]->isLimited;
- deferred[i] = NULL; // fast. kick() will skip empty entries later.
- debugs(5, 5, HERE << afd->conn << " x" << afd->isLimited);
+ uint64_t abandonedClients = 0;
+ for (unsigned int i = 0; i < deferred_.size() && afd->isLimited > 0; ++i) {
+ if (deferred_[i] == afd) {
+ -- deferred_[i]->isLimited;
+ deferred_[i] = NULL; // fast. kick() will skip empty entries later.
+ debugs(5, 5, afd->conn << " x" << afd->isLimited);
+ ++abandonedClients;
}
}
+ debugs(5,4, "Abandoned " << abandonedClients << " client TCP SYN by
closing socket: " << afd->conn);
}
void
@@ -38,13 +42,14 @@
// TODO: this could be optimized further with an iterator to search
// looking for first non-NULL, followed by dumping the first N
// with only one shift()/pop_front operation
+ // OR, by reimplementing as a list instead of Vector.
- debugs(5, 5, HERE << " size=" << deferred.size());
- while (deferred.size() > 0 && fdNFree() >= RESERVED_FD) {
+ debugs(5, 5, "size=" << deferred_.size());
+ while (deferred_.size() > 0 && fdNFree() >= RESERVED_FD) {
/* NP: shift() is equivalent to pop_front(). Giving us a FIFO queue. */
- TcpAcceptor *temp = deferred.shift();
- if (temp != NULL) {
- debugs(5, 5, HERE << " doing one.");
+ TcpAcceptor::Pointer temp = deferred_.shift();
+ if (temp.valid()) {
+ debugs(5, 5, "doing one.");
-- temp->isLimited;
temp->acceptNext();
break;
=== modified file 'src/comm/AcceptLimiter.h'
--- src/comm/AcceptLimiter.h 2011-05-13 08:13:01 +0000
+++ src/comm/AcceptLimiter.h 2013-03-01 02:08:12 +0000
@@ -2,12 +2,11 @@
#define _SQUID_SRC_COMM_ACCEPT_LIMITER_H
#include "Array.h"
+#include "comm/TcpAcceptor.h"
namespace Comm
{
-class TcpAcceptor;
-
/**
* FIFO Queue holding listener socket handlers which have been activated
* ready to dupe their FD and accept() a new client connection.
@@ -18,6 +17,16 @@
* removeDead - used only by Comm layer ConnAcceptor to remove themselves when
dying.
* kick - used by Comm layer when FD are closed.
*/
+/* TODO this algorithm can be optimized further:
+ *
+ * 1) reduce overheads by only pushing one entry per port to the list?
+ * use TcpAcceptor::isLimited as a flag whether to re-list when kick()'ing
+ * or to NULL an entry while scanning the list for empty spaces.
+ * Side effect: TcpAcceptor->kick() becomes allowed to pull off multiple
accept()'s in bunches
+ *
+ * 2) re-implement as a list instead of vector?
+ * storing head/tail pointers for fast push/pop and avoiding the whole shift()
overhead
+ */
class AcceptLimiter
{
@@ -26,10 +35,10 @@
static AcceptLimiter &Instance();
/** delay accepting a new client connection. */
- void defer(Comm::TcpAcceptor *afd);
+ void defer(const TcpAcceptor::Pointer &afd);
/** remove all records of an acceptor. Only to be called by the
ConnAcceptor::swanSong() */
- void removeDead(const Comm::TcpAcceptor *afd);
+ void removeDead(const TcpAcceptor::Pointer &afd);
/** try to accept and begin processing any delayed client connections. */
void kick();
@@ -38,7 +47,7 @@
static AcceptLimiter Instance_;
/** FIFO queue */
- Vector<Comm::TcpAcceptor*> deferred;
+ Vector<TcpAcceptor::Pointer> deferred_;
};
}; // namepace Comm
=== modified file 'src/comm/TcpAcceptor.h'
--- src/comm/TcpAcceptor.h 2011-01-31 11:50:28 +0000
+++ src/comm/TcpAcceptor.h 2013-03-01 03:20:26 +0000
@@ -1,17 +1,11 @@
#ifndef SQUID_COMM_TCPACCEPTOR_H
#define SQUID_COMM_TCPACCEPTOR_H
-#include "base/AsyncCall.h"
+#include "base/AsyncJob.h"
+#include "base/CbcPointer.h"
#include "base/Subscription.h"
-#include "CommCalls.h"
#include "comm_err_t.h"
#include "comm/forward.h"
-#include "comm/TcpAcceptor.h"
-#include "ip/Address.h"
-
-#if HAVE_MAP
-#include <map>
-#endif
namespace Comm
{
@@ -32,6 +26,9 @@
*/
class TcpAcceptor : public AsyncJob
{
+public:
+ typedef CbcPointer<Comm::TcpAcceptor> Pointer;
+
private:
virtual void start();
virtual bool doneAll() const;
=== modified file 'src/tests/stub_libcomm.cc'
--- src/tests/stub_libcomm.cc 2013-01-21 07:15:09 +0000
+++ src/tests/stub_libcomm.cc 2013-03-01 01:31:33 +0000
@@ -7,8 +7,8 @@
#include "comm/AcceptLimiter.h"
Comm::AcceptLimiter dummy;
Comm::AcceptLimiter & Comm::AcceptLimiter::Instance() STUB_RETVAL(dummy)
-void Comm::AcceptLimiter::defer(Comm::TcpAcceptor *afd) STUB
-void Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor *afd) STUB
+void Comm::AcceptLimiter::defer(const Comm::TcpAcceptor::Pointer &afd) STUB
+void Comm::AcceptLimiter::removeDead(const Comm::TcpAcceptor::Pointer &afd)
STUB
void Comm::AcceptLimiter::kick() STUB
#include "comm/Connection.h"
