From: Amos Jeffries <squid3@treenet.co.nz>
Date: Wed, 25 Nov 2015 04:21:40 +0000 (-0800)
Subject: Cleanup: Refactor ConnStateData pipeline handling
X-Git-Tag: SQUID_4_0_3~5
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=83b40f50d8bd32363c7c43d0260b23e3206fbdc1;p=thirdparty%2Fsquid.git

Cleanup: Refactor ConnStateData pipeline handling

This refactors the request pipeline management API to use std::list
instead of a custom linked-list with accessors spread over both
ConnStateData and ClientSocketContext.

To do this a new class Pipeline is created with methods wrapping
std::list API and extending it slightly to meet the HTTP/1.1 pipeline
behaviours and perform basic stats gathering. The pipeline management
methods and state variables are moved inside this class.

ClientSocketContext was performing several layering violations in
relation to ConnStateData when one transaction ended and the next needed
starting. Treating the pipeline properly as a std::list forced removal
of that violation.

* actions for starting or resuming a transaction on the connection are
now moved to ConnStateData::kick(). Which gets called after each
transaction completes.
 - with some further cleanup it can be called at any point the
ConnStateData needs to resume processing. However, that is left out of
scope for this patch.

* the ClientSocketContext scope now ends when the finished() method is
used to mark completion of these contexts transactions. Which will mark
itself done and de-register from the Pipeline queue. The ConnStateData
kick() method still needs to be called to resume other transactions
processing.

* the queue is now holding RefCounted Pointers. So that the
ClientSocketContext destructor no longer needs to be careful of
registrations, and the queue entries are guaranteed to still exist while
queued.

* The old freeAllContexts() and notifyAllContexts(int) members of
ConnStateData have been combined into Pipeline::terminateAll(int).



The ClientSocketContext and ConnStateData documentation is updated to
describe what they do in regards to connection and transaction processing.


Initial testing revealed CONNECT tunnels always being logged as ABORTED.
This turns out to be techincally correct, since the only way a tunnel
can finish is for client or server to just close the connection.
However, it is not right to log these as abnormal aborts. Instead, I
have now made the context be finished() just prior to the
TunnelStateData being destroyed. That way normal closure should show up
only as TUNNEL, but timeouts and I/O errors should still be recorded as
abnormal.


Two potential bugs have been highlighted:

* The on_unsupported_protocol handling function appears to be a bit
broken. It pop()'s contexts off the pipeline directly without going
through the proper finished() process to release their state data. I
have highlighted that with an XXX and comment.

* The ssl-bump handling logic switching to TLS begins with a terminateAll(0)
run on all active contexts. It does not check whether there is any existing
pipeline of requests waiting to be processed. And the action prematurely
purges the bumped CONNECT message context, which should be closed properly
and logged as successful.
---

83b40f50d8bd32363c7c43d0260b23e3206fbdc1