Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for HTTP/2 non-indexed header fields #5249

Open
mjduijn opened this issue Jan 16, 2019 · 3 comments
Open

Support for HTTP/2 non-indexed header fields #5249

mjduijn opened this issue Jan 16, 2019 · 3 comments

Comments

@mjduijn
Copy link

mjduijn commented Jan 16, 2019

What version of gRPC are you using?

08efd97 (HEAD on Jan 3, 2019)

What did you expect to see?

Ability to set header names outside of a-z, -

We are trying to send a signature value as gRPC metadata, the value changes with every message.
Metadata is translated to headers in HTTP/2 which in our case, due to HPACK, the client, server and reverse proxy store in the dynamic table for header compression.
As a result the dynamic table size will increase until its full (by default up to 4096 octets).
When it's entries are removed from the dynamic table in FIFO order to make space for the new entries.
This is undesirable because of the increase in memory use in our proxy and because after X requests all the headers are resent.

HTTP/2 supports non-indexed headers, which as far as I can gather from the docs, do not get stored in the dynamic table.
You signal a non-indexed header with the first four bytes of the header key being '0001' or '0000'.
However these are non-valid ASCII characters and thus do not get through the validity check.
I'm sure the validity check is there for a reason, and I very much realize we're in an edge case here.

Would it be possible at all to bypass the check or is this something that shouldn't be done for compatibility reasons?

@carl-mastrangelo
Copy link
Contributor

@mjduijn A couple things:

  1. The 0001 is part of the HPACK spec and doesn't interact with the actual content. HPACK can encode arbitrary binary data. Only HTTP (as in RFC 7230, not 7540). Also, this is not part of any public API in gRPC (or Netty, for that matter).

  2. Resending all the headers was not a significant source of latency in my benchmarks (though I cannot comment on the memory usage that you bring up). In gRPC deadlines are encoded as relative timeouts and included in the header values. This means almost every rpc with a deadline is going to use a different header value. I tried making a change to gRPC similar to your suggesting to make deadlines non-indexed. This had no measurable impact on latency; it was in the noise.

  3. Netty has a SensitivityDetector that you could try hacking in to our code. This would allow you to see if there is an appreciable difference in memory usage.

@mjduijn
Copy link
Author

mjduijn commented Jan 17, 2019

Thank you for investigating and putting things in perspective.

I had not realized deadlines also cause constantly changing header fields, this completely nullifies the memory usage argument.
Since our clients will be using deadlines, the dynamic table will grow to its limit for every connection anyway, thus adding another constantly changing header shouldn't have any impact.

As for the latency, I had a hard time measuring latency spikes but I can tell you that the number of bytes in the frame increases from 150 to 275 every 25th request (approximately).
This means that the difference in bytes on the wire is negligible and supports your findings.

Because of the not-significant difference, this seems like a perfectly valid approach performance-wise. Since you tagged the issue I am assuming you would like to keep it open, please don't do so just for my sake.

@carl-mastrangelo
Copy link
Contributor

A legitimate reason to support non-indexed fields would be security sensitive fields, such as encryption keys or passwords. Using the SensitivityDetector as mentioned above would accomplish this, but we don't have a way of wiring it down yet. I added the label, though it's not likely I will be able to work on this in the near future.

If you can show there is a measurable performance impact (latency, memory, or otherwise), or if you can show it would be unsafe to use gRPC without this feature, we can raise the priority of supporting non-indexed fields.

@ejona86 ejona86 added this to the Unscheduled milestone Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants