Skip to content

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Nov 22, 2021

Map known headers to QPACK static table. Write name id instead of full value when header is in the static table.

return H3StaticTable.ContentLength0;
case KnownHeaderType.Date:
return H3StaticTable.Date;
case KnownHeaderType.Cookie:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consider adding the un-"known" response headers given the methodology described in the quickwg QPACK static table appendix.

I also think the following headers only make sense on the request: Cookie, Authorization, UserAgent, Referer, Origin, IfModifiedSince, IfNoneMatch, Method, Accept, AcceptEncoding, AcceptLanguage, Range, IfRange and UpgradeInsecureRequests

Do we think there would be much value in trying to use the static header values too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't add more known headers because there is a limit to how many we can have. Eventually, the bit flags will be too large for 64bits.

I will look at using the static table for values in a future PR. It will require greater changes.

Copy link
Member

@Tratcher Tratcher Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think the following headers only make sense on the request: Cookie, Authorization, UserAgent, Referer, Origin, IfModifiedSince, IfNoneMatch, Method, Accept, AcceptEncoding, AcceptLanguage, Range, IfRange and UpgradeInsecureRequests

Yes, remove request headers from this list, if only to make it easier to reason about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For HPACK, we have the complete list:

internal static int GetResponseHeaderStaticTableId(KnownHeaderType responseHeaderType)
{
switch (responseHeaderType)
{
case KnownHeaderType.CacheControl:
return H2StaticTable.CacheControl;
case KnownHeaderType.Date:
return H2StaticTable.Date;
case KnownHeaderType.TransferEncoding:
return H2StaticTable.TransferEncoding;
case KnownHeaderType.Via:
return H2StaticTable.Via;
case KnownHeaderType.Allow:
return H2StaticTable.Allow;
case KnownHeaderType.ContentType:
return H2StaticTable.ContentType;
case KnownHeaderType.ContentEncoding:
return H2StaticTable.ContentEncoding;
case KnownHeaderType.ContentLanguage:
return H2StaticTable.ContentLanguage;
case KnownHeaderType.ContentLocation:
return H2StaticTable.ContentLocation;
case KnownHeaderType.ContentRange:
return H2StaticTable.ContentRange;
case KnownHeaderType.Expires:
return H2StaticTable.Expires;
case KnownHeaderType.LastModified:
return H2StaticTable.LastModified;
case KnownHeaderType.AcceptRanges:
return H2StaticTable.AcceptRanges;
case KnownHeaderType.Age:
return H2StaticTable.Age;
case KnownHeaderType.ETag:
return H2StaticTable.ETag;
case KnownHeaderType.Location:
return H2StaticTable.Location;
case KnownHeaderType.ProxyAuthenticate:
return H2StaticTable.ProxyAuthenticate;
case KnownHeaderType.RetryAfter:
return H2StaticTable.RetryAfter;
case KnownHeaderType.Server:
return H2StaticTable.Server;
case KnownHeaderType.SetCookie:
return H2StaticTable.SetCookie;
case KnownHeaderType.Vary:
return H2StaticTable.Vary;
case KnownHeaderType.WWWAuthenticate:
return H2StaticTable.WwwAuthenticate;
case KnownHeaderType.AccessControlAllowOrigin:
return H2StaticTable.AccessControlAllowOrigin;
case KnownHeaderType.ContentLength:
return H2StaticTable.ContentLength;
default:
return -1;
}

Do you want to trim the HPACK list as well? Which items?

Or leave both HPACK and QPACK lists as is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok, leave it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me that Http2HeadersEnumerator.GetResponseHeaderStaticTableId() only handles response headers. I don't see request headers like Cookie, Authorization or any of the rest in there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I'll make the change to remove them in another PR.

@halter73
Copy link
Member

I like the test reorganization under Http1/Http2/Http3

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the status compression is out of date: #38604

@JamesNK JamesNK merged commit 4656f15 into main Nov 23, 2021
@JamesNK JamesNK deleted the jamesnk/http3-writestaticheaders branch November 23, 2021 20:05
@ghost ghost added this to the 7.0-preview1 milestone Nov 23, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants