Re: [PATCH] refactor ConnStateData pipeline handling

On 05/08/2014 11:17 AM, Amos Jeffries wrote:
> 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.

> +/// a pipeline of received HTTP/1 request transactions
> +class Pipeline {

Please avoid mixing "request" and "transaction" together. In most
contexts, "transaction" is a (request, response) pair. IIRC,
ClientSocketContext represents just a request, not transaction as a
whole. At some point, we even had active responses without
ClientSocketContext (I do not know whether that is still true).

I consider the scope of ClientSocketContext and, hence, Pipeline
important for this project. For example, if ClientSocketContext is about
a request, not a response, then we should get rid of it when we switch
to the next request, but if ClientSocketContext is about transaction,
then we should keep it until response processing is complete. I think it
is the former.

If you agree, please avoid using "transaction" throughout new comments.

> + /// Transactions parsed from the connection but possibly incomplete.
> + /// For HTTP/1 these are handled as a FIFO queue of request messages.
> + std::queue<ClientSocketContextPointer> Q;

Non-static class data members should start with a lower case letter.
Please avoid single-letter member names. "requests" would work OK here,
I guess.

> + /// get the first transaction context in the pipeline
> + /// regardless of whether it is completed or not
> + ClientSocketContextPointer get() const;

"get" is a rather generic name for a "pipeline" object. I suggest to use
"front" instead.

> + /// Tell everybody about the err, and abort all waiting transactions.
> + /// A value of 0 just aborts.
> + void notifyAll(const int xerrno);

Note that this is not what the old notifyAllContexts() did. You are
converting a logging-related method into request termination method. You
can do that, of course, but renaming the method would be wise.
terminateAll() may work better.

> +/// propagates abort event to all contexts
> +/// while clearing the queue
> +void
> +Pipeline::notifyAll(int xerrno)

I am guessing a description in .cc file is not needed because we
describe this public method in .h file, as we should. The "to all
contexts" part of the .cc description is lying because the event is not
propagated to "finished" contexts.

Please note that the old method was used to propagate two different
events: timeout and connection abort. In your comments, it is better to
talk about "error event" propagation rather than "abort event"
propagation because the former includes both aborts and timeouts.

> + while (!Q.empty()) {
> + ClientSocketContextPointer context = Q.front();
> + Q.pop();
> + // if the transaction is already complete, just drop it.
> + if (!context->doneAll()) {

Do we have to keep "completed" contexts stored in the pipeline? Can we
simplify things by equating pipeline membership with "completeness"? In
other words, whenever a context object becomes "complete" or "finished",
we remove it from the pipeline. This will simplify and clarify many
things, including avoid concerns that the new Pipeline::empty() method
returns false for an essentially empty pipeline that does not have any
requests waiting.

Is removing connRegistered_ (and using pipeline registration instead)
doable?

> +void
> +Pipeline::clearCompleted()
> +{
> + // find next incomplete request in the queue (if any)
> + while (!Q.empty()) {
> + if (!Q.front()->doneAll())
> + break;
> + debugs(33, 3, "Pipeline " << (void*)this << " drop " << Q.front());
> + Q.pop();
> + }
> +}

> + // find next incomplete request in the queue (if any)
> + pipeline.clearCompleted();

This method does not find anything from the caller point of view. It
removes leading "completed" contexts.

> - freeAllContexts();
...
> + if (!pipeline.empty()) {
> + // BUG? starting SSL with a pipeline queue of requests still waiting for non-SSL replies.
> + pipeline.notifyAll(0); // abort the lot.
> + }

Please remove the emptiness check. The notifyAll() call should handle
empty queue correctly itself, and the caller code should not imply
otherwise (because it gets copied/misinterpreted/etc). If you remove the
check, please update the "BUG?" description to be specific to a
non-empty pipeline

FWIW, I suggest using XXX instead of BUG to minimize the number of
various internal markers with very similar meanings:

> [rousskov_at_dut src]$ fgrep -w -RI BUG . | fgrep -v '"BUG' | wc -l
> 18
> [rousskov_at_dut src]$ fgrep -w -RI XXX . | wc -l
> 421

And yes, the existing code is already not 100% consistent:

> ./http.cc: \bug XXX BUG?

With three bug markers at once? It must be! :-)

> + /// whether this context can be discarded
> + bool doneAll() const { return !connRegistered_;}
> +

I do not think we should conflate the new method with
AsyncJob-specific/internal doneAll(), to avoid the wrong implication
that ClientSocketContext is a job. How about finished()?

> 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.

And if it does not, this is not an SslBump bug. Regular CONNECT tunnels
ought to do the same.

> * 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.

I am not sure ClientSocketContext is the right place for bumping
operations because we are not talking about a temporary detour to
process a single HTTP request. In fact, the last request processing is
over when we start bumping. Instead, we are switching into a special
tunnel mode. The best place is probably neither ConnStateData nor
ClientSocketContext but an SslBump-equivalent of TunnelStateData.

I do not volunteer to change this now, especially since Peek and Splice
is fiddling with the same code, but we should keep this problem in mind.

> 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.

It may be even more complicated/messy. IIRC, ClientSocketContext objects
[in the pipeline] represent client requests and it may be possible that
the "pipeline" is empty, but some active response(s?) are still going
on. Code comments imply this is possible, but I am not sure whether
those comments are still valid.

HTH,

Alex.
Received on Fri May 09 2014 - 20:56:49 MDT