HTTP: add Frame type enumeration

[yadij]

No description provided.

[yadij]

Type IDs per current HTTP specifications for HTTP/2, HTTP/3 and extensions.
HTTP/1 message sections can be interpreted in terms of these IDs
as needed when logic is redesigned to handle any HTTP version.

[rousskov]

Adding unused code requires justification. A PR that adds nothing but unused code requires serious justification. I do not see any justification in current PR title/description. I also see no good reason to add this unused code right now¹, so I cannot suggest or guess what that justification might be.

There are other problems with this PR, but I am focusing on the primary one for now.

Footnotes

1. We should not start adding HTTP/2+ support bits until we pay off major technical debt standing in the way of proper HTTP/2+ support (e.g., Client Streams API and Server class). ↩

[yadij]

Adding unused code requires justification.
...

1. We should not start adding HTTP/2+ support bits until we pay off major technical debt standing in the way of proper HTTP/2+ support (e.g., Client Streams API and Server class).

I believe that refactoring/cleanup you refer to should be done with the understanding that Squid needs to add support for HTTP/2+ versions. As such, incorporating the concepts from those latest versions to interpret and handle the existing HTTP/1 messages - rather than refactoring with only regard to HTTP/1 needs.

We have already added the "stream" concept and using it as pertains to HTTP/1. Next relevant is this "frame" concept for type-code identifying the parts of HTTP messages being handled.

[rousskov]

I believe that refactoring/cleanup you refer to should be done with the understanding that Squid needs to add support for HTTP/2+ versions.

That understanding already exists, so that precondition is already satisfied. If you disagree, then I would certainly take quality cleanup unaffected by that understanding than no cleanup at all! After all, that technical debt affects existing HTTP/1 support as well -- it is not just in the way of HTTP/2+ support. It is in the way of many things.

As such, incorporating the concepts from those latest versions to interpret and handle the existing HTTP/1 messages - rather than refactoring with only regard to HTTP/1 needs.

While some concepts can and should be incorporated, frame type identifiers (i.e. the subject of this PR) is not really a "concept", and we have no use for them today AFAICT. We can add them when/if the situation changes.

We have already added the "stream" concept and using it as pertains to HTTP/1. Next relevant is this "frame" concept for type-code identifying the parts of HTTP messages being handled.

If "stream concept" is existing Client Streams code, then that concept should be removed/replaced. We cannot build anything (worth building) on top of it. I am not aware of any primary Squid code that can benefit from introducing a frame concept at least until that technical debt is resolved.

[yadij]

I believe that refactoring/cleanup you refer to should be done with the understanding that Squid needs to add support for HTTP/2+ versions.

That understanding already exists, so that precondition is already satisfied.

Your initial response to this PR despite my #1853 (comment) indicates that there are still some differences to work out.

FTR, the ConnStateData refactoring has proven to be the most controversial. So I have pulling these small pieces out of it to make some progress. The type added by this PR is unused right now having it present in the form HTTP/2 needs allows any of us to make more steps towards that HTTP/1 refactoring.

If you disagree, then I would certainly take quality cleanup unaffected by that understanding than no cleanup at all! After all, that technical debt affects existing HTTP/1 support as well -- it is not just in the way of HTTP/2+ support. It is in the way of many things.

As such, incorporating the concepts from those latest versions to interpret and handle the existing HTTP/1 messages - rather than refactoring with only regard to HTTP/1 needs.

While some concepts can and should be incorporated, frame type identifiers (i.e. the subject of this PR) is not really a "concept", and we have no use for them today AFAICT. We can add them when/if the situation changes.

You have a circular argument there. This addition does not exist because old code does not use it. HTTP/2+ code cannot go in because these pre-requisites do not exist. Ergo, Squid cannot ever gain new code for HTTP/2+ support.

We have already added the "stream" concept and using it as pertains to HTTP/1. Next relevant is this "frame" concept for type-code identifying the parts of HTTP messages being handled.

If "stream concept" is existing Client Streams code, then that concept should be removed/replaced.

No, I am referring to Http::Stream.

We cannot build anything (worth building) on top of it. I am not aware of any primary Squid code that can benefit from introducing a frame concept at least until that technical debt is resolved.

For future HTTP/2+ versions the Client Stream is better replaced with an API passing Frame of type DATA instead of StoreIOBuffer blocks around. To start on that API replacement we need the definitions for the new types that are being passed around. The simplest of those types is this PR addition.

[rousskov]

I believe that refactoring/cleanup you refer to should be done with the understanding that Squid needs to add support for HTTP/2+ versions.

That understanding already exists, so that precondition is already satisfied.

... there are still some differences to work out.

Very true: While this particular precondition has been satisfied long time ago, there are many other refactoring/cleanup issues that we approach very differently.

the ConnStateData refactoring has proven to be the most controversial. So I have pulling these small pieces out of it to make some progress.

FWIW, I still do not see this PR as progress. The discussion so far has not helped.

The type added by this PR is unused right now having it present in the form HTTP/2 needs allows any of us to make more steps towards that HTTP/1 refactoring.

Correct "HTTP/1 refactoring" should not need this type AFAICT. That is one of the primary reasons I am against adding this unused code right now. I do not want to reject a followup refactoring PR that injects HTTP/2+ frame IDs into HTTP/1 code. And I do not want to reject another PR that adds HTTP/2+ code prior to required refactoring of existing code.

As such, incorporating the concepts from those latest versions to interpret and handle the existing HTTP/1 messages - rather than refactoring with only regard to HTTP/1 needs.

While some concepts can and should be incorporated, frame type identifiers (i.e. the subject of this PR) is not really a "concept", and we have no use for them today AFAICT. We can add them when/if the situation changes.

You have a circular argument there. This addition does not exist because old code does not use it. HTTP/2+ code cannot go in because these pre-requisites do not exist. Ergo, Squid cannot ever gain new code for HTTP/2+ support.

My argument is not circular because "these prerequisites" do not include "this addition". We need to fix several major problems with the current foundation first and only then start adding HTTP/2+ bits. The former does not require (and can be harmed by) the latter. Very roughly speaking, the next steps should follow a plan similar to this:

1. Remove ESI
2. Replace Client Streams
3. Probably remove Server::pipeline
4. Merge Server back into ConnStateData
5. Reassess the need for further necessary prerequisite steps and perform them (if any)
6. Start adding HTTP/2+ bits

For future HTTP/2+ versions the Client Stream is better replaced with an API passing Frame of type DATA instead of StoreIOBuffer blocks around. To start on that API replacement we need the definitions for the new types that are being passed around. The simplest of those types is this PR addition.

An API that passes HTTP message body blocks does not need (and should not know about) frame types.