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. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
