[squid-dev] [PATCH] Http::StreamContext refactoring

[Alex Rousskov]

On 01/14/2016 05:53 AM, Amos Jeffries wrote:

> The ClientSocketContext is renamed and shuffled to Http::StreamContext.

This class does not belong to the Http namespace. It represents the user
side of a master transaction. While Squid historically uses classes
named after HTTP to represent various master transaction aspects (e.g.,
HttpRequest and HttpReply), we should not fall into this trap when
naming new classes or renaming the old ones. As you know, the user side
of a master transaction may correspond to any of these (at least):

  * An HTTP stream (user coming from an http_port or alike).
  * An FTP stream (user coming from an ftp_port).
  * An internal stream (various Squid-originated downloads).

The "Context" suffix adds nothing to the meaning of the new name. It is
essentially noise, like most "State", "Data", and "Object" suffixes. I
suggest dropping it.

Since there are many Squid things that can be considered "streams", we
do need some distinguishing qualifier for the new name. As you know, we
are avoiding old "client/server side" terminology so ClientStream would
not work. If this thing is going to be specific to user traffic, we
could call it UserStream. If you think it will be eventually used for
peer/origin transactions as well, then a different qualifier is needed.


The renaming/moving part of the patch scope changes lots of code. It is
very unfortunate that you have decided to combine the above
functionality-preserving polishing with the functionality changes below.
The former makes it very difficult to see the latter. If I had enough
guts, I would -1 this patch for this reason alone. Renaming/moving
changes should have been done separately!


> Pipeline class is updated to use the ID number to manage its contents
> rather than Pointer value matching. It is also updated to drop the
> HTTP/1 specific assumptions within the Pipeline implementation. As a
> behavioural requirement the sequential flow is now left for the Server
> and ClientHttpRequest Jobs to ensure correctness.


The new Pipeline::popById() method is a bad idea IMO: Linear search is
wrong for both HTTP/1 and HTTP/2.


> The old "client-side" classes are now split along the following scopes:


> * ::Server / ConnStateData/ *::Server
>  - the AsynJob managing I/O on a client connection.
> 
> * ClientHttpRequest
>  - the AsyncJob operating transaction internal behaviours and Calls.
> Will make use of ConnStateData and Http::StreamContext to perform its
> duties (has not yet been refactored to do so cleanly yet).
> 
> * Http::StreamContext
>  - the state data for a transaction. Refcounted and used by the Server
> and ClientHttpRequest Jobs for their mutual data storage.


As I said earlier, I will have to come back to this part later, but
perhaps you can already clarify whether this patch has actually changed
any of the above (besides renaming)?


Thank you,

Alex.