Item 1 in recent branch commit
21f1082 correctly identified the
Connection sharing problem, but its solution (i.e. let ConnOpener clone
the passed Connection) was not only somewhat expensive but buggy:
ConnOpener assumed that cloneDestinationDetails() is the right cloning
method, but that method missed Connection properties like tos and nfmark
(at least). Another method -- cloneIdentDetails() would copy those, but
calling that method in non-IDENT contexts would look weird, and even
that method could have missed something (now or in the future).
The core problem here was not in the correct cloning method selection,
but that ConnOpener is not in a position to select at all! ConnOpener
should be using/forwarding all the provided Connection details, not
guessing which ones are relevant to the _caller_. After that became
clear, we had two choices:
A. ConnOpener clones everything. This is a simple, safe choice. However,
it means double-cloning: First, the caller would need to clone to
copy just the right set of Connection members (or effectively clear
the irrelevant ones). And then ConnOpener would clone again to break
the Connection ownership link with poorly implemented callers. In
some cases, that cloning was already following ResolvedPeers cloning,
resulting in triple cloning of each used destination on the common
forwarding path! Such cloning is both expensive and complicates
triage by creating long chains of Connection IDs.
B. ConnOpener clones nothing; ConnOpener creator is responsible for
supplying ConnOpener with a non-owned Connection object (that does
not need to be cloned by ConnOpener). This is a clean, efficient,
forward-looking solution but it is a bit less safe because the
compiler will not identify buggy creators that share a Connection
object they own with ConnOpener.
After weighing all these impossible-to-compare factors, we went with B.
Implementing B required adjusting several callers that were still
passing the Connections they owned to ConnOpener.
Also, after comparing the needs of existing and newly added ("clone
everything") Connection cloning method callers, we decided there is no
need to maintain three different methods. All existing callers should be
fine with a single method because none of them suffers from "extra"
copying of members that others need. Right now, the new cloneProfile()
method copies everything except FD and a few special-purpose members
(with documented reasons for not copying).
entry->mem_obj->checkUrlChecksum();
#endif
- GetMarkingsToServer(cause.getRaw(), *dest);
+ const auto conn = dest->cloneProfile();
+ GetMarkingsToServer(cause.getRaw(), *conn);
- // ConnOpener modifies its destination argument so we reset the source port
- // in case we are reusing the destination already used by our predecessor.
- dest->local.port(0);
++n_tries;
typedef CommCbMemFunT<HappyConnOpener, CommConnectCbParams> Dialer;
AsyncCall::Pointer callConnect = asyncCall(48, 5, attempt.callbackMethodName,
Dialer(this, attempt.callbackMethod));
const time_t connTimeout = dest->connectTimeout(fwdStart);
- Comm::ConnOpener *cs = new Comm::ConnOpener(dest, callConnect, connTimeout);
- if (!dest->getPeer())
+ Comm::ConnOpener *cs = new Comm::ConnOpener(conn, callConnect, connTimeout);
+ if (!conn->getPeer())
cs->setHost(host_);
+ attempt.path = dest; // but not the being-opened conn!
attempt.connWait.start(cs, callConnect);
AsyncJob::Start(cs);
while (++pathsToSkip < paths_.size() && !paths_[pathsToSkip].available) {}
}
- const auto cleanPath = path.connection->cloneDestinationDetails();
+ const auto cleanPath = path.connection->cloneProfile();
return PeerConnectionPointer(cleanPath, found - paths_.begin());
}
AsyncJob("Comm::ConnOpener"),
host_(NULL),
temporaryFd_(-1),
- conn_(c->cloneDestinationDetails()),
+ conn_(c),
callback_(handler),
totalTries_(0),
failRetries_(0),
deadline_(squid_curtime + static_cast<time_t>(ctimeout))
{
debugs(5, 3, "will connect to " << c << " with " << ctimeout << " timeout");
+ assert(conn_); // we know where to go
+
+ // Sharing a being-modified Connection object with the caller is dangerous,
+ // but we cannot ban (or even check for) that using existing APIs. We do not
+ // want to clone "just in case" because cloning is a bit expensive, and most
+ // callers already have a non-owned Connection object to give us. Until the
+ // APIs improve, we can only check that the connection is not open.
+ assert(!conn_->isOpen());
}
Comm::ConnOpener::~ConnOpener()
}
Comm::ConnectionPointer
-Comm::Connection::cloneDestinationDetails() const
+Comm::Connection::cloneProfile() const
{
- const ConnectionPointer c = new Comm::Connection;
- c->setAddrs(local, remote);
- c->peerType = peerType;
- c->flags = flags;
- c->peer_ = cbdataReference(getPeer());
- debugs(5, 5, this << " made " << c);
- assert(!c->isOpen());
- return c;
-}
+ const ConnectionPointer clone = new Comm::Connection;
+ auto &c = *clone; // optimization
+
+ /*
+ * Copy or excuse each data member. Excused members do not belong to a
+ * Connection configuration profile because their values cannot be reused
+ * across (co-existing) Connection objects and/or are tied to their own
+ * object lifetime.
+ */
+
+ c.setAddrs(local, remote);
+ c.peerType = peerType;
+ // fd excused
+ c.tos = tos;
+ c.nfmark = nfmark;
+ c.nfConnmark = nfConnmark;
+ // COMM_ORPHANED is not a part of connection opening instructions
+ c.flags = flags & ~COMM_ORPHANED;
+ if (*rfc931) // optimization
+ memcpy(c.rfc931, rfc931, sizeof(rfc931));
+
+#if USE_SQUID_EUI
+ // These are currently only set when accepting connections and never used
+ // for establishing new ones, so this copying is currently in vain, but,
+ // technically, they can be a part of connection opening instructions.
+ c.remoteEui48 = remoteEui48;
+ c.remoteEui64 = remoteEui64;
+#endif
-Comm::ConnectionPointer
-Comm::Connection::cloneIdentDetails() const
-{
- auto c = cloneDestinationDetails();
- c->tos = tos;
- c->nfmark = nfmark;
- c->nfConnmark = nfConnmark;
- c->startTime_ = startTime_;
- return c;
+ // id excused
+ c.peer_ = cbdataReference(getPeer());
+ // startTime_ excused
+ // tlsHistory excused
+
+ debugs(5, 5, this << " made " << c);
+ assert(!c.isOpen());
+ return clone;
}
void
/** Clear the connection properties and close any open socket. */
virtual ~Connection();
- /// Create a new (closed) IDENT Connection object based on our from-Squid
- /// connection properties.
- ConnectionPointer cloneIdentDetails() const;
+ /// To prevent accidental copying of Connection objects that we started to
+ /// open or that are open, use cloneProfile() instead.
+ Connection(const Connection &&) = delete;
- /// Create a new (closed) Connection object pointing to the same destination
- /// as this from-Squid connection.
- ConnectionPointer cloneDestinationDetails() const;
+ /// Create a new closed Connection with the same configuration as this one.
+ ConnectionPointer cloneProfile() const;
/// close the still-open connection when its last reference is gone
void enterOrphanage() { flags |= COMM_ORPHANED; }
virtual ScopedId codeContextGist() const override;
virtual std::ostream &detailCodeContext(std::ostream &os) const override;
-private:
- /** These objects may not be exactly duplicated. Use cloneIdentDetails() or
- * cloneDestinationDetails() instead.
- */
- Connection(const Connection &c);
-
- /** These objects may not be exactly duplicated. Use cloneIdentDetails() or
- * cloneDestinationDetails() instead.
- */
- Connection & operator =(const Connection &c);
-
public:
/** Address/Port for the Squid end of a TCP link. */
Ip::Address local;
Ident::Close(const CommCloseCbParams ¶ms)
{
IdentStateData *state = (IdentStateData *)params.data;
+ // XXX: A Connection closure handler must update its Connection object.
state->deleteThis("connection closed");
}
IdentStateData *state = (IdentStateData *)data;
state->connWait.finish();
- // Finalize the details and start owning the supplied connection (so that it
- // is not orphaned if this function bails early). As a (tiny) optimization
- // or perhaps just diff minimization, the close handler is added later, when
- // we know we are not bailing. This delay is safe because
- // comm_remove_close_handler() forgives missing handlers.
+ // Start owning the supplied connection (so that it is not orphaned if this
+ // function bails early). As a (tiny) optimization or perhaps just diff
+ // minimization, the close handler is added later, when we know we are not
+ // bailing. This delay is safe because comm_remove_close_handler() forgives
+ // missing handlers.
assert(conn); // but may be closed
- assert(state->conn);
- assert(!state->conn->isOpen());
+ assert(!state->conn);
state->conn = conn;
if (status != Comm::OK) {
state->hash.key = xstrdup(key);
// copy the conn details. We do not want the original FD to be re-used by IDENT.
- state->conn = conn->cloneIdentDetails();
+ const auto identConn = conn->cloneProfile();
// NP: use random port for secure outbound to IDENT_PORT
- state->conn->local.port(0);
- state->conn->remote.port(IDENT_PORT);
+ identConn->local.port(0);
+ identConn->remote.port(IDENT_PORT);
// build our query from the original connection details
state->queryMsg.init();
hash_join(ident_hash, &state->hash);
AsyncCall::Pointer call = commCbCall(30,3, "Ident::ConnectDone", CommConnectCbPtrFun(Ident::ConnectDone, state));
- const auto connOpener = new Comm::ConnOpener(state->conn, call, Ident::TheConfig.timeout);
+ const auto connOpener = new Comm::ConnOpener(identConn, call, Ident::TheConfig.timeout);
state->connWait.start(connOpener, call);
AsyncJob::Start(connOpener);
}
// active transfer: open a data connection from Squid to client
typedef CommCbMemFunT<Server, CommConnectCbParams> Dialer;
AsyncCall::Pointer callback = JobCallback(17, 3, Dialer, this, Ftp::Server::connectedForData);
- const auto cs = new Comm::ConnOpener(dataConn, callback,
+ const auto cs = new Comm::ConnOpener(dataConn->cloneProfile(), callback,
Config.Timeout.connect);
dataConnWait.start(cs, callback);
AsyncJob::Start(cs);
Http::StreamPointer context = pipeline.front();
Must(context->http);
Must(context->http->storeEntry() != NULL);
+ // TODO: call closeDataConnection() to reset data conn processing?
} else {
// Finalize the details and start owning the supplied connection.
assert(params.conn);
#include "comm/Connection.h"
Comm::Connection::Connection() STUB
Comm::Connection::~Connection() STUB
-Comm::ConnectionPointer Comm::Connection::cloneIdentDetails() const STUB_RETVAL(nullptr)
-Comm::ConnectionPointer Comm::Connection::cloneDestinationDetails() const STUB_RETVAL(nullptr)
+Comm::ConnectionPointer Comm::Connection::cloneProfile() const STUB_RETVAL(nullptr)
void Comm::Connection::close() STUB
void Comm::Connection::noteClosure() STUB
CachePeer * Comm::Connection::getPeer() const STUB_RETVAL(NULL)
projects / thirdparty / squid.git / commitdiff
