]> git.ipfire.org Git - thirdparty/squid.git/commit - src/Transients.cc
Cleanup: Refactor ConnStateData pipeline handling
authorAmos Jeffries <squid3@treenet.co.nz>
Wed, 25 Nov 2015 04:21:40 +0000 (20:21 -0800)
committerAmos Jeffries <squid3@treenet.co.nz>
Wed, 25 Nov 2015 04:21:40 +0000 (20:21 -0800)
commit83b40f50d8bd32363c7c43d0260b23e3206fbdc1
tree0b6b8e973b681e28253da69b7f62a129139bdd0e
parentfe3bffac6a232ce22d2a1a14aea46bdf9c7697e4
parent82696c26e7b8565ced9b0698e5f58dc7b2fe6dee
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.
src/Makefile.am
src/client_side.cc
src/stat.cc