On 18/06/2013 3:33 a.m., Alex Rousskov wrote:
On 06/09/2013 06:50 AM, Amos Jeffries wrote:
New patch attached for review.
This excludes the XXX/TODO and pipeline parts, most of which are already
merged and the remainder being deferred until a later update.
Thank you for addressing my concerns related to pipelining changes.
Please note that the mk2 patch is missing the new MasterXaction class
being added. The notes below are for the changes actually present in the
patch.
Oh rats. mk3 attached.
Hopefully you will have time to review this today. I have incorporated
your latest items (elided) in this as the intended final version.
BTW, is is probably a bad idea to use cbdata-protected PortCfg. Ideally,
when reconfiguration changes the port, it should not be invalidated,
causing all sorts of assertions for pending transactions using that
port. We should use reference counting instead (with some additional API
to mark no-longer-configured ports), but that change is outside your
project scope, of course.
The fact that the CfgPortPointer is a CbcPointer is unfortunate factor
of its use elsewhere in the code. If we can make it a RefCount that
would be good, but a separate patch. All I'm aiming at here is to ensure
that the available Pointer is used consistently by the new code ready
for that fix.
=== modified file 'src/CommCalls.h'
...
+ /// Transaction which this call is part of.
+ MasterXaction::Pointer xaction;
It feels wrong to place that object inside every Comm call. A Comm call
is a very low-level event. What would it do with a Master Transaction?
It would not even know which part of the Master Transaction corresponds
to the call! In fact, there are Comm calls that are not associated with
a master transaction. Let's not try to cram master transactions
everywhere just because it is needed somewhere. It will force us to
either create fake master transactions or deal with possibly nil master
transaction pointers.
Right now the TcpAcceptor AsyncJob is using it in AcceptCbParams to
pass the new transaction out to its child.
Storing a master transaction pointer may be appropriate for
AcceptCbParams because you want all accepts, even UDP and SMTP ones, to
be associated with some master transaction (an important and not-obvious
design decision on its own!).
I am envisioning that the CloseCbParams will need to use it to pass it
to the Comm layer close handlers,
Why? I would imagine that any job handling a connection closure should
already have a pointer to the master transaction because that job had to
deal with the corresponding _open_ connection (associated with that
master transaction) at some point.
Quite. I'm having second thoughts about my initial reasons -
particularly that the Jobs or state objects which are receiving those
close and I/O calls likely have a local referance to the Xaction anyway.
This mk3 removes it from those CommCall until such time as it becomes
necessary to add there.
- for each of those
components to write the stats accounting details into the xaction
data. For now it is a NULL pointer on these parameters.
The only stats I can think of they would be able to update are "I/Os per
master transaction" stats, which we do not collect now, and probably for
a good reason (they would be mostly meaningless because I/Os differ too
much depending on which connection socket or disk file descriptor they
are being performed).
Until there is code that actually collects these questionable stats,
let's keep this new master tranasction pointer confined to the accept
callbacks (if you insist on that). We can always move the master
transaction pointer member to the parent callback data class if we
decide it is needed there for one reason or another.
I was thinking more along the lines of error counters. But that is more
global in scope anyway, or can be added at the other end of the Call
(handler) where the Xaction is probably available anyway.
We will need to adjust the description of the MasterXaction class
accordingly. For now, I suggest adding three important pieces of
information:
* What is a "client transaction"? Should we define that as anything
worth logging into access.log? Or does that extend to ICP clients? SNMP?
* What event starts/creates a master transaction?
I am thinking these conditions create a new transaction:
1. New TCP connection, New UDP connection, New SCTP connection, etc. etc.
2. Parsing a new request on an existing persistent connection of any
above type.
3. starting a Squid internally generated request
We get into some tricky semantics with ssl-bump and ESI that still
need to be worked out.
I could not review the corresponding changes since the MasterXact class
was missing from the mk2 patch. They are very important though IMO.
It also demonstrates to you and others the mechanism for passing
between Jobs.
True, but if feels like the wrong mechanism to me. While it may sound
convenient to make a MasterXaction object a universally-accessible
global (essentially), I think it will create more problems than it will
solve because not all async calls and not all async jobs are related to
a master transaction. This mechanism does not reflect reality well and,
hence, is likely to fail long-term. Let's not introduce it until we
really need it.
I don't mean it to be universally accepted. Just for the main pathways
for request/response handling linking client-side and server-side.
There will be components such as the IDENT, DNS, WHOIS and ICAP which
maintain their own xaction state objects which only get linked into this
as sub-objects in those Jobs or Call handlers where this object
coincides with those ones.
Please do not commit all those XXXs/TODOs though. After the
MasterXact skeleton is finished and committed (this thread), just post
the new MasterXact-using code as you have time to work on it.
Do you not think it better to mark the missing API alterations for
others to be aware of and maybe chose to join in working on?
IIRC, some of those added XXXs and TODOs were incorrect IMO, but I would
prefer not to delay this change by discussing them here and now. Thank
you for removing them.
Sorry for not being able to review this earlier,
Alex.
Amos
=== modified file 'src/CommCalls.cc'
--- src/CommCalls.cc 2012-08-28 13:00:30 +0000
+++ src/CommCalls.cc 2013-06-17 16:47:48 +0000
@@ -40,8 +40,17 @@
/* CommAcceptCbParams */
CommAcceptCbParams::CommAcceptCbParams(void *aData):
- CommCommonCbParams(aData)
-{
+ CommCommonCbParams(aData), xaction()
+{
+}
+
+void
+CommAcceptCbParams::print(std::ostream &os) const
+{
+ CommCommonCbParams::print(os);
+
+ if (xaction != NULL)
+ os << ", " << xaction->id;
}
/* CommConnectCbParams */
=== modified file 'src/CommCalls.h'
--- src/CommCalls.h 2013-01-02 23:40:49 +0000
+++ src/CommCalls.h 2013-06-17 16:48:23 +0000
@@ -5,6 +5,7 @@
#include "base/AsyncJobCalls.h"
#include "comm_err_t.h"
#include "comm/forward.h"
+#include "MasterXaction.h"
/* CommCalls implement AsyncCall interface for comm_* callbacks.
* The classes cover two call dialer kinds:
@@ -89,6 +90,11 @@
{
public:
CommAcceptCbParams(void *aData);
+
+ void print(std::ostream &os) const;
+
+ /// Transaction which this call is part of.
+ MasterXaction::Pointer xaction;
};
// connect parameters
=== modified file 'src/Makefile.am'
--- src/Makefile.am 2013-06-07 04:35:25 +0000
+++ src/Makefile.am 2013-06-07 04:41:21 +0000
@@ -422,6 +422,8 @@
LogTags.h \
lookup_t.h \
main.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
Mem.h \
mem.cc \
mem_node.cc \
@@ -1251,6 +1253,8 @@
HttpRequestMethod.cc \
int.h \
int.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
SquidList.h \
SquidList.cc \
mem_node.cc \
@@ -1488,6 +1492,8 @@
internal.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
multicast.h \
multicast.cc \
mem_node.cc \
@@ -1668,6 +1674,8 @@
int.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
MemBuf.cc \
MemObject.cc \
mem_node.cc \
@@ -1904,6 +1912,8 @@
internal.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
Mem.h \
mem.cc \
mem_node.cc \
@@ -2152,6 +2162,8 @@
internal.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
MemBlob.cc \
MemBuf.cc \
MemObject.cc \
@@ -2397,6 +2409,8 @@
ipcache.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
MemBlob.cc \
MemBuf.cc \
MemObject.cc \
@@ -2687,6 +2701,8 @@
internal.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
multicast.h \
multicast.cc \
mem_node.cc \
@@ -2860,6 +2876,8 @@
int.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
Mem.h \
mem.cc \
mem_node.cc \
@@ -3085,6 +3103,8 @@
RequestFlags.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
MemObject.cc \
StoreSwapLogData.cc \
StoreIOState.cc \
@@ -3271,6 +3291,8 @@
int.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
Mem.h \
mem.cc \
MemBuf.cc \
@@ -3655,6 +3677,8 @@
internal.cc \
SquidList.h \
SquidList.cc \
+ MasterXaction.cc \
+ MasterXaction.h \
multicast.h \
multicast.cc \
Mem.h \
=== added file 'src/MasterXaction.cc'
--- src/MasterXaction.cc 1970-01-01 00:00:00 +0000
+++ src/MasterXaction.cc 2013-06-07 04:15:51 +0000
@@ -0,0 +1,4 @@
+#include "squid.h"
+#include "MasterXaction.h"
+
+InstanceIdDefinitions(MasterXaction, "MXID_");
=== added file 'src/MasterXaction.h'
--- src/MasterXaction.h 1970-01-01 00:00:00 +0000
+++ src/MasterXaction.h 2013-06-17 16:49:30 +0000
@@ -0,0 +1,38 @@
+#ifndef SQUID_SRC_MASTERXACTION_H
+#define SQUID_SRC_MASTERXACTION_H
+
+#include "anyp/forward.h"
+#include "base/CbcPointer.h"
+#include "base/InstanceId.h"
+#include "base/Lock.h"
+#include "comm/forward.h"
+
+/** Master transaction details.
+ *
+ * This class holds pointers to all the state
+ * generated during the processing of a client
+ * transaction.
+ *
+ * Transactions are begun with a request message from
+ * a new client connection and initial request message, or
+ * an existing connection pipelined request message, or
+ * an internally (Squid) generated request message.
+ */
+class MasterXaction : public RefCountable
+{
+public:
+ typedef RefCount<MasterXaction> Pointer;
+
+ /// transaction ID.
+ InstanceId<MasterXaction> id;
+
+ /// the listening port which originated this transaction
+ AnyP::PortCfgPointer squidPort;
+
+ /// the client TCP connection which originated this transaction
+ Comm::ConnectionPointer tcpClient;
+
+ // TODO: add state from other Jobs in the transaction
+};
+
+#endif /* SQUID_SRC_MASTERXACTION_H */
=== modified file 'src/client_side.cc'
--- src/client_side.cc 2013-06-07 08:49:36 +0000
+++ src/client_side.cc 2013-06-17 16:21:48 +0000
@@ -245,8 +245,6 @@
char *skipLeadingSpace(char *aString);
static void connNoteUseOfBuffer(ConnStateData* conn, size_t byteCount);
-static ConnStateData *connStateCreate(const Comm::ConnectionPointer &client,
AnyP::PortCfg *port);
-
clientStreamNode *
ClientSocketContext::getTail() const
{
@@ -3330,23 +3328,37 @@
io.conn->close();
}
-ConnStateData *
-connStateCreate(const Comm::ConnectionPointer &client, AnyP::PortCfg *port)
+ConnStateData::ConnStateData(const MasterXaction::Pointer &xact) :
+ AsyncJob("ConnStateData"),
+#if USE_SSL
+ sslBumpMode(Ssl::bumpEnd),
+ switchedToHttps_(false),
+ sslServerBump(NULL),
+#endif
+ stoppedSending_(NULL),
+ stoppedReceiving_(NULL)
{
- ConnStateData *result = new ConnStateData;
-
- result->clientConnection = client;
- result->log_addr = client->remote;
- result->log_addr.applyMask(Config.Addrs.client_netmask);
- result->in.buf = (char *)memAllocBuf(CLIENT_REQ_BUF_SZ,
&result->in.allocatedSize);
- result->port = cbdataReference(port);
+ pinning.host = NULL;
+ pinning.port = -1;
+ pinning.pinned = false;
+ pinning.auth = false;
+ pinning.zeroReply = false;
+ pinning.peer = NULL;
+
+ // store the details required for creating more MasterXaction objects as
new requests come in
+ clientConnection = xact->tcpClient;
+ port = cbdataReference(xact->squidPort.get());
+ log_addr = xact->tcpClient->remote;
+ log_addr.applyMask(Config.Addrs.client_netmask);
+
+ in.buf = (char *)memAllocBuf(CLIENT_REQ_BUF_SZ, &in.allocatedSize);
if (port->disable_pmtu_discovery != DISABLE_PMTU_OFF &&
- (result->transparent() || port->disable_pmtu_discovery ==
DISABLE_PMTU_ALWAYS)) {
+ (transparent() || port->disable_pmtu_discovery ==
DISABLE_PMTU_ALWAYS)) {
#if defined(IP_MTU_DISCOVER) && defined(IP_PMTUDISC_DONT)
int i = IP_PMTUDISC_DONT;
- if (setsockopt(client->fd, SOL_IP, IP_MTU_DISCOVER, &i, sizeof(i)) < 0)
- debugs(33, 2, "WARNING: Path MTU discovery disabling failed on "
<< client << " : " << xstrerror());
+ if (setsockopt(clientConnection->fd, SOL_IP, IP_MTU_DISCOVER, &i,
sizeof(i)) < 0)
+ debugs(33, 2, "WARNING: Path MTU discovery disabling failed on "
<< clientConnection << " : " << xstrerror());
#else
static bool reported = false;
@@ -3358,33 +3370,39 @@
}
typedef CommCbMemFunT<ConnStateData, CommCloseCbParams> Dialer;
- AsyncCall::Pointer call = JobCallback(33, 5, Dialer, result,
ConnStateData::connStateClosed);
- comm_add_close_handler(client->fd, call);
+ AsyncCall::Pointer call = JobCallback(33, 5, Dialer, this,
ConnStateData::connStateClosed);
+ comm_add_close_handler(clientConnection->fd, call);
if (Config.onoff.log_fqdn)
- fqdncache_gethostbyaddr(client->remote, FQDN_LOOKUP_IF_MISS);
+ fqdncache_gethostbyaddr(clientConnection->remote, FQDN_LOOKUP_IF_MISS);
#if USE_IDENT
if (Ident::TheConfig.identLookup) {
ACLFilledChecklist identChecklist(Ident::TheConfig.identLookup, NULL,
NULL);
- identChecklist.src_addr = client->remote;
- identChecklist.my_addr = client->local;
+ identChecklist.src_addr = xact->tcpClient->remote;
+ identChecklist.my_addr = xact->tcpClient->local;
if (identChecklist.fastCheck() == ACCESS_ALLOWED)
- Ident::Start(client, clientIdentDone, result);
+ Ident::Start(xact->tcpClient, clientIdentDone, this);
}
#endif
- clientdbEstablished(client->remote, 1);
+ clientdbEstablished(clientConnection->remote, 1);
- result->flags.readMore = true;
- return result;
+ flags.readMore = true;
}
/** Handle a new connection on HTTP socket. */
void
httpAccept(const CommAcceptCbParams ¶ms)
{
- AnyP::PortCfg *s = static_cast<AnyP::PortCfg *>(params.data);
+ MasterXaction::Pointer xact = params.xaction;
+ AnyP::PortCfgPointer s = xact->squidPort;
+
+ if (!s.valid()) {
+ // it is possible the call or accept() was still queued when the port
was reconfigured
+ debugs(33, 2, "HTTP accept failure: port reconfigured.");
+ return;
+ }
if (params.flag != COMM_OK) {
// Its possible the call was still queued when the client disconnected
@@ -3402,7 +3420,7 @@
++ incoming_sockets_accepted;
// Socket is ready, setup the connection manager to start using it
- ConnStateData *connState = connStateCreate(params.conn, s);
+ ConnStateData *connState = new ConnStateData(xact);
typedef CommCbMemFunT<ConnStateData, CommTimeoutCbParams> TimeoutDialer;
AsyncCall::Pointer timeoutCall = JobCallback(33, 5,
@@ -3651,7 +3669,7 @@
/**
* A callback function to use with the ACLFilledChecklist callback.
- * In the case of ACCES_ALLOWED answer initializes a bumped SSL connection,
+ * In the case of ACCESS_ALLOWED answer initializes a bumped SSL connection,
* else reverts the connection to tunnel mode.
*/
static void
@@ -3693,7 +3711,14 @@
static void
httpsAccept(const CommAcceptCbParams ¶ms)
{
- AnyP::PortCfg *s = static_cast<AnyP::PortCfg *>(params.data);
+ MasterXaction::Pointer xact = params.xaction;
+ const AnyP::PortCfgPointer s = xact->squidPort;
+
+ if (!s.valid()) {
+ // it is possible the call or accept() was still queued when the port
was reconfigured
+ debugs(33, 2, "HTTPS accept failure: port reconfigured.");
+ return;
+ }
if (params.flag != COMM_OK) {
// Its possible the call was still queued when the client disconnected
@@ -3711,7 +3736,7 @@
++incoming_sockets_accepted;
// Socket is ready, setup the connection manager to start using it
- ConnStateData *connState = connStateCreate(params.conn, s);
+ ConnStateData *connState = new ConnStateData(xact);
if (s->flags.tunnelSslBumping) {
debugs(33, 5, "httpsAccept: accept transparent connection: " <<
params.conn);
@@ -4310,24 +4335,6 @@
CBDATA_CLASS_INIT(ConnStateData);
-ConnStateData::ConnStateData() :
- AsyncJob("ConnStateData"),
-#if USE_SSL
- sslBumpMode(Ssl::bumpEnd),
- switchedToHttps_(false),
- sslServerBump(NULL),
-#endif
- stoppedSending_(NULL),
- stoppedReceiving_(NULL)
-{
- pinning.host = NULL;
- pinning.port = -1;
- pinning.pinned = false;
- pinning.auth = false;
- pinning.zeroReply = false;
- pinning.peer = NULL;
-}
-
bool
ConnStateData::transparent() const
{
=== modified file 'src/client_side.h'
--- src/client_side.h 2013-05-30 14:58:49 +0000
+++ src/client_side.h 2013-06-17 16:51:25 +0000
@@ -187,8 +187,7 @@
{
public:
-
- ConnStateData();
+ explicit ConnStateData(const MasterXaction::Pointer &xact);
~ConnStateData();
void readSomeData();
@@ -273,6 +272,7 @@
AsyncCall::Pointer closeHandler; /*The close handler for pinned server
side connection*/
} pinning;
+ /// Squid listening port details where this connection arrived.
AnyP::PortCfg *port;
bool transparent() const;
=== modified file 'src/comm/TcpAcceptor.cc'
--- src/comm/TcpAcceptor.cc 2013-06-03 14:05:16 +0000
+++ src/comm/TcpAcceptor.cc 2013-06-07 04:41:21 +0000
@@ -33,6 +33,7 @@
*/
#include "squid.h"
+#include "anyp/PortCfg.h"
#include "base/TextException.h"
#include "client_db.h"
#include "comm/AcceptLimiter.h"
@@ -46,6 +47,7 @@
#include "fde.h"
#include "globals.h"
#include "ip/Intercept.h"
+#include "MasterXaction.h"
#include "profiler/Profiler.h"
#include "SquidConfig.h"
#include "SquidTime.h"
@@ -286,8 +288,10 @@
if (theCallSub != NULL) {
AsyncCall::Pointer call = theCallSub->callback();
CommAcceptCbParams ¶ms = GetCommParams<CommAcceptCbParams>(call);
+ params.xaction = new MasterXaction;
+ params.xaction->squidPort = static_cast<AnyP::PortCfg*>(params.data);
params.fd = conn->fd;
- params.conn = newConnDetails;
+ params.conn = params.xaction->tcpClient = newConnDetails;
params.flag = flag;
params.xerrno = errcode;
ScheduleCallHere(call);
