[PATCH] refactor ConnStateData pipeline handling

From: Amos Jeffries <squid3_at_treenet.co.nz>
Date: Fri, 09 May 2014 05:17:17 +1200

This refactors the request pipeline management API to use std::queue
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::queue 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::queue forced removal
of that violation.

 * actions for starting or resuming a transaction on the connection are
now moved to ConnStateData::kick() and simply called by
ClientSocketContext when its finished.

 * the ClientSocketContext scope now ends at the point of marking itself
done. For now it still calls the new kick() method to let ConnStateData
move on to the next transaction. Moving that to a better location will
require deep analysis of the client_side.cc to find such a place.

 * queue de-registration of completed contexts is now done by
ConnStateData by calling pipeline::clearCompleted() as part of its
preparations for handling a next transaction. A doneAll() accessor is
added to ClientSocketContext for the Pipeline class to determine whether
the context can be dropped or needs to be processed further.

* 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 (even if completed).

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

**********

This refactor reveals a potential bug in ssl-bump. For now I have marked
and retained the buggy behaviour to limit scope of this change.

What should be happening when bumping:

 * when a CONNECT is encountered the read(2) operations should "pause"
until the pipeline is drained up to the CONNECT.
 - I believe the request processing code already does this in a
roundabout way.

 * the actions of bumping should be to emit the 200 OK response, do the
bump, then adjust the CONNECT request context such that doneAll()
returns true, then kick() the ConnStateData to start processing the
HTTPS traffic.
 - this sequence also hints that the bumping operations should be
driven/performed by ClientSocktContext, rather than ConnStateData where
they are now. Both have direct access to the client socket so that is
not a problem, and the context has access to getConn() to change its
state details.

NP: at the point when the CONNECT is bumped if the pipeline holds
*anything* other than a single CONNECT request context which is already
doneAll()==true, the ssl-bumping MUST NOT be started. Wait until that
condition is met before switching to HTTPS.
 - The ssl-bump code instead is unconditionally clearing the entire
pipeline and switching to HTTPS immediately.

NP: the removal/free/notify of the CONNECT request context does not have
to be done by the bumping code. Just mark it for doneAll() so that when
the ssl-bumped traffic is resumed with a ConnStateData::kick() the
context will be cleaned up.
 This also means that the bumping code can guarantee for itself that the
CONNECT data still exists as pipeline.get() output up until the point
where the first encrypted request is processed by kick().

Amos

Received on Thu May 08 2014 - 17:17:34 MDT

This archive was generated by hypermail 2.2.0 : Sat May 10 2014 - 12:00:12 MDT