[squid-dev] Care and feeding of ConnStateData

[Alex Rousskov]

On 07/07/2016 04:16 PM, Amos Jeffries wrote:
> On 8/07/2016 7:22 a.m., Alex Rousskov wrote:
>> On 07/06/2016 10:52 PM, Amos Jeffries wrote:
>>> On 7/07/2016 10:24 a.m., Alex Rousskov wrote:
>>>> Q4. What to do with the existing src/servers/Server.h?
>>>> * It does not fully encapsulate connection-managing code of its kids
>>>> (its stated usage).

>>> ... those changes to make it so got blocked in audit.

>> Whether some future changes will or will not address this part of the
>> problem is pretty much irrelevant to the problem existence (it only
>> confirms that existence if you will).

> Its pretty significant that you blocked a class being implemented to
> match its design and now use the fact that its not happeed as a reason
> against the class existing at all.

I understand that the combination looks bad, especially if you phrase it
like that, but the actual situation is far more nuanced:

- I really doubt that there is some wonderful pending patch that would
fully address this part of the problem.

- If I block a version of a patch, there are several means to unblock
progress. It is unfair to imply that I made progress impossible.

- Resolving one problem in the wrong place often creates two new ones.
Who knows how many more bullets would appear on this list if that
wonderful change were to be committed.

- Removing this bullet/problem requires adding more code to Server.h,
which means more changes to undo. This bullet is about a Server.h
imperfection. It is just meant to make us like that code less, so it is
even easier to accept the small burden of undoing its addition. Because
of the other problems I have mentioned, even a "perfect" Server.h class
would need to be merged back into ConnStateData!



> Our disgareement is about which class of the two is "broken".

I do not think there can be any serious disagreement about that. Both
classes are clearly broken, and we may even agree on what the end result
should be. There can only be a disagreement regarding the correct way to
get to that end result.

Normally, I would resist wasting a lot of discussion time if others
insist on doing things the wrong way as long as the end result is
correct. In this particular case, the amount of [current and especially
future] pain and suffering associated with this wrong way warrants
spending more time in hope to correct the situation.


> IMO a class lacking some of its needed API (::Server) is far less broken
> than a class with a lot of differently scoped bits tacked on all over
> the place (ConnStateData).

That decision logic is valid in many cases but it is inapplicable here.
We are not deciding which of the two alternative implementations of X to
adopt!

Instead, we have a single complex working implementation of X (including
a few misplaced features that X should not be concerned with) and an
essentially empty/unused X parent. Moving most of X code to its parent
class to isolate those misplaced features does not solve any serious
problems but does introduce new ones. The only sane solution is to
remove those misplaced features from X and continue to use and improve X
(under a new name).


>>> The two grandchildren in trunk we planned to make direct children of it
>>> (yes we, all three of us at the time agreed). The intermediate kid being
>>> the ConnStateData mess still *temporarily* in the middle of the hierarchy.

>> *If* I agreed on moving Server code out of ConnStateData instead of
>> moving non-Server code out of ConnStateData, then I made a major mistake!
>>
>> What anybody did or did not say years ago is secondary to the current
>> discussion,

> It is the status-quo design basis on which the current discussion is
> taking place. So yeah, it is relevant.

Agreed. I did not say it is irrelevant. I said it is secondary. *If* we
agreed on the wrong path in the past, we ought to re-evaluate our
approach now. I actually believe we agreed on the same path that I am
advocating now [but that is secondary].



>> but I know that in 2014 I said pretty much the same thing I
>> am saying now[4]: "Keep ConnStateData as is for now, until somebody
>> volunteers to extract HTTP code from it and place the extracted HTTP
>> code into servers/HttpServer, moving the remaining generic code into
>> src/servers/Server and src/http/".
>>
> 
> Yes, exactly. As the person who for all those years has been attemping
> to *do* that work...

Sorry, I am not sure what you mean. Are you implying that you are the
only person who did that work? And that being the only person doing that
work somehow automatically makes your work correct? Both implications
are false IMO, but perhaps I am misinterpreting what you are implying by
that statement.


>> In other words:
>>
>>   1. Extract HTTP server-specific code from ConnStateData.
>>
>>   ...once #1 is done, ConnStateData will only have generic code left...


> Assuming the extraction happens and audit does not block it. Which was
> my point above.

Step #1 remains correct whether a specific patch is or is not blocked.
No patches were blocked because they were extraction attempts. If
anything was blocked, it was blocked for _other_ reasons. Mentioning
those blocked-for-other-reasons patches here does not make any of us
right or wrong.


> I *have* been proposing exactly those changes and getting blocks thrown
> up constantly preventing stuff being moved out of ConnStateData.

Why are you saying that here? Does the fact that some of your changes
were blocked somehow justify Server.h existence? I do not think so. If
anything, it is a [very weak] vote against Server.h!

I can explain or defend specific blocking decisions. I cannot explain or
defend all of them at once, here, because they were all different,
specific to the changes being proposed.


> Usually on grounds that as a parial-measure its not good enough, 

The question is "Was the patch good enough to be committed?". If the
correct answer is "no", then the patch was blocked correctly and
mentioning that bad patch here is counterproductive to your argument.

If the correct answer is "yes", then you should work on unblocking those
correct changes. We are all reasonable people without absolute powers so
it is possible to unblock something if it was wrongly blocked. However,
this thread is the wrong place to do that because the unblocking
discussion has to be specific to the change being proposed.


> like
> ConnStateData as a whole (huge and messy as it is) must be retained and
> perfected in a single step before anything is acceptible.

That requirement would be clearly absurd/indefensible. Nothing was or
can be blocked based on that bogus requirement [that never existed].


>>   2. Move that remaining generic code into src/servers/Server and src/http/.
>>
>> We all have been doing step #1 since then, but we are not yet ready for
>> step #2 because ConnStateData still has HTTP server-specific code in it.


> Apart from auditing all I'm seeing from Factory is more and more
> non-Server things being added to ConnStateData. First it was 1xx message
> handling, then ssl-bump, then all the FTP translation logics, then
> Downloader (thanks for reversing on that one).

If you think that those who introduced FTP and HTTP Server child classes
did not contribute enough to the ConnStateData untangling, and if you
are careless or mean enough to throw in SslBump feature added five years
before the untangling discussion started or Downloader that was posted
as a _preview_ and did _not_ add much to ConnStateData even in that
unfinished form, then I am not going to try to change your mind.

Moreover, whether XYZ has contributed to ConnStateData cleanup is
completely irrelevant to this discussion!


>> When we are ready for step #2, the "move" will be exactly that -- we
>> will be moving files and renaming classes to match their new location.
>> There is no time in this plan where there are two Server classes, one
>> called "Server" and one called "ConnStateData"!


> The .h and .cc files are shared with ClientSocketContext and
> ClientHttpRequest. Which are more or less all of BB2 between them.

Yes, I know.


> Since a fairly big portion of the ConnStateData logic is also BB2 scope
> stuff done in the wrong class it just gets a rename in-place during
> stage #1.

Sorry, I do not know what you mean by "it" and "gets a rename in-place"
in this context.


> So in order to get any given file ready for a rename/move. We either

Just to avoid misunderstanding: Renaming/moving classes is separate from
renaming/moving files. Both will be done, but the decision on what to
rename/move (and where) is separate for each file and each class.


> a) shuffle _all_ the BB2 scoped stuff out of its currently locations
> (regardless of whether its in ConnStateData or not). OR,
> 
> b) shuffle the (relatively smaller) pieces of ConnStateData out to
> Server.* during step #1 and the rename/move is to change the file into
> whatever the BB2 component name is.

I disagree. Doing (a) is not required to rename ConnStateData class to
Server. As far as (a) and renaming are concerned, we only need to remove
BB2 stuff from the ConnStateData class, and there is not much of it
left, as revamped Downloader and old ESI classes prove. I will post
specific suggestions after the Downloader work is completed.

And option (b) appears to assume that ConnStateData class is mostly BB2,
when, in fact, it is not.


>>>> * It forces new code to add pure virtual methods that ConnStateData then
>>>> overrides and implements, reminding me of the old Store class that
>>>> slowly accumulated dozens of such methods because we needed to add
>>>> something that one of the Store kids implemented.

>>> This is backwards.

>> Exactly! The ConnStateData split forces us to do things backwards.

> No I meant your proposal is backwards. Moving a mountain instead of a
> molehill.

I am not moving a mountain. I am renaming it after moving a few
misplaced boulders elsewhere. You, on the other hand, are splitting one
mountain into two, building various temporary ladders to cross the
divide while moving boulders from one side of the divide to another.

Will the mountain need a split sometime in the future? We do not know
yet. What we do know that the split is not needed now, and that it
creates problems.



>> The correct way to remove code misplaced in ConnStateData is to move
>> that misplaced code (rather than to split ConnStateData in two and then
>> move everything else into the second class, leaving misplaced code in
>> ConnStateData, as misplaced as it was when we started).


> What remains will be BB2.

Perhaps we define BB2 boundaries differently, but I believe that
removing BB2 from the ConnStateData class is a relatively small change.
I can volunteer to make that change after Downloader work is completed.
It should not take long.


> You are calling to revert the changes that separate the Server logics
> from the BB2+Server logics still in ConnStateData.

Not really. I am essentially calling to replace the letters "Server"
with the letters "ConnStateData" and move/merge ~7 simple methods
currently in Server.cc. You can even keep the Server.cc file if you
think that keeping those methods in a separate file is valuable. Even
keeping the declarations in the Server.h file is possible if you really
insist.


>> I certainly regret if something I said earlier was misinterpreted as the
>> need to create a Server class that slowly accumulates code already
>> properly placed in ConnStateData. I hope the above arguments prove that
>> we must do the opposite. Pretty much everything in design and code
>> refactoring is a matter of opinion, but this choice is as close to being
>> "formally provable" as it gets.

> You seem to have forgotten the reason why you agreed to it after (3?)
> months of discussion last time around. 

And what was that reason?


> I dont think that was ever
> written down exactly so can't point at it, but I do remember you
> agreeing to it early one morning for me.

Sounds like a "you agreed to it then, so it must be right, even though
it certainly looks wrong now, and we do not know why you agreed to it in
the first place" argument. I do not think that argument is good enough
here: We are not lawyers arguing whether the plaintiff has signed a
piece of paper. Our decisions should be based primarily on merits, not
protocol (and the protocol part also works against you -- I just do not
want to play that part of the game!).


> I certainly regret that you think anything at all of the current logic
> should be retained in ConnStateData.

I do not think that. There are known/marked HTTP- and FTP-specific
pieces in ConnStateData that should be moved. There are a couple of BB2
pieces as well. And other stuff, I am sure (this is not a comprehensive
list).


> Whichever way we go what the ::Server needs is:

... Snipped to avoid discussing complex design issues irrelevant for
this thread. I agree with some of the items you listed, but not
necessarily all of it, and there may be missing items...


> Some things to note about that list vs where we are today:
> 
> * the design of ::Server presenting generic I/O does not exist in
> ConnStateData. Never has. What it has is logics for Http1::Server child
> to read and write directly to the socket. That is making all the child
> protocols except HTTP/1 more complicated than they should be.

I do not follow. Nobody is currently proposing to delete the "generic"
I/O code in Server.cc or its interface in Server.h. ConnStateData does
not even override those methods so the interface will continue to be
used as is and the methods will require virtually no modifications to
move them back to the ConnStateData class.



> * that ssl-bump, PROXY protocol, HTTP/1 vs h2 vs FTP, HTTP/1 1XX status
> handling etc are all BB2 or Server-child specific activities. The child
> duty in some places may be simplified down to passing an appropriate
> Parser/Packer object for the ::Server to use via its Parser/Packable API
> - but that is still child specific.

I may disagree about some of it but it is not important right now IMO.


>  We dont need to go near the existing Server vs ConnStateData separation
> to do that shuffling. In fact ConnStateData is acting somewhat as a TODO
> list of things yet to be shuffled.

Here is how your argument/plan looks from my side: "Ignore the problems
and pains associated with having two Server classes that you have
extensively documented. Let me commit lots of changes that you have
strongly disagreed with in the past. Silently suffer for a few years
until I complete the complex work still ahead. Then you will enjoy the
'perfect Server class' nirvana."

Needless to say, "move a few methods back to ConnStateData" sounds like
a much better plan to me!

Please note that my plan does not prevent you from accomplishing
everything you want to accomplish and does not really increase your
suffering by much -- you may still treat those few previously identified
by you methods as "true Server" methods, use them as needed, and even
add more to their collection.


Thank you,

Alex.