codec-http2: Improve h1 to h2 header conversion #7399

mosesn · 2017-11-10T22:13:37Z

Motivation:

Netty could handle "connection" or "te" headers more gently when
converting from http/1.1 to http/2 headers. Http/2 headers don't
support single-hop headers, so when we convert from http/1.1 to http/2,
we should drop all single-hop headers. This includes headers like
"transfer-encoding" and "connection", but also the headers that
"connection" points to, since "connection" can be used to designate
other headers as single-hop headers. For the "te" header, we can more
permissively convert it by just dropping non-conforming headers (ie
non-"trailers" headers) which is what we do for all other headers when
we convert.

Modifications:

Add a new blacklist to the http/1.1 to http/2 conversion, which is
constructed from the values of the "connection" header, and stop
throwing an exception when a "te" header is passed with a non-"trailers"
value. Instead, drop all values except for "trailers". Add unit tests
for "connection" and "te" headers when converting from http/1.1 to http/2.

Result:

This will improve the h2c upgrade request, and also conversions from
http/1.1 to http/2. This will simplify implementing spec-compliant
http/2 servers that want to share code between their http/1.1 and http/2
implementations.

[Fixes #7355]

normanmaurer · 2017-11-10T23:13:39Z

codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java

@@ -410,19 +412,30 @@ public static Http2Headers toHttp2Headers(HttpHeaders inHeaders, boolean validat
        return out;
    }

+    private static CharSequenceMap<AsciiString> toLowercaseMap(List<String> values) {
+        CharSequenceMap<AsciiString> result = new CharSequenceMap<AsciiString>();


should we use the constructor that takes the arraySizeHint based on values.size() ?

normanmaurer · 2017-11-10T23:14:33Z

codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java

@@ -410,19 +412,30 @@ public static Http2Headers toHttp2Headers(HttpHeaders inHeaders, boolean validat
        return out;
    }

+    private static CharSequenceMap<AsciiString> toLowercaseMap(List<String> values) {
+        CharSequenceMap<AsciiString> result = new CharSequenceMap<AsciiString>();
+        for (CharSequence value : values) {


consider using an "old for" loop if the List implements RandomAccess to reduce GC pressure (as the foreach thingy you use here will always create an Iterator)

While it creates an iterator, it can be allocated on the stack. (Whether it actually is allocated on the stack is a different discussion.)

@ejona86 we saw GC issues caused by this in the past so better safe then sorry ;)

looks like it's a linked list: https://github.com/netty/netty/blob/4.1/codec/src/main/java/io/netty/handler/codec/DefaultHeaders.java#L163

ejona86 · 2017-11-11T00:06:36Z

codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java

@@ -410,19 +412,30 @@ public static Http2Headers toHttp2Headers(HttpHeaders inHeaders, boolean validat
        return out;
    }

+    private static CharSequenceMap<AsciiString> toLowercaseMap(List<String> values) {
+        CharSequenceMap<AsciiString> result = new CharSequenceMap<AsciiString>();
+        for (CharSequence value : values) {


Don't you need also need to split value on , and trim whitespace?

mosesn · 2017-11-13T19:16:56Z

@ejona86 @normanmaurer PTAL

ejona86 · 2017-11-15T20:59:07Z

codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java

+                    }
+                    result.add(lowerCased.subSequence(start, lowerCased.length(), false).trim(), EMPTY_STRING);
+                } else {
+                    result.add(lowerCased, EMPTY_STRING);


Do we need to worry about trimming this? HTTP parsers will generally naturally remove whitespace (but aren't guaranteed to), but that doesn't happen in HTTP/2 (from what I've seen; maybe Netty does).

ejona86 · 2017-11-15T21:13:17Z

codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java

+                        start = index + 1;
+                    } while (start < lowerCased.length() &&
+                             (index = lowerCased.forEachByte(start, value.length() - start, FIND_COMMA)) != -1);
+                    if (start >= lowerCased.length()) {


It doesn't seem like this should be special-cased or that all empty strings should be handled (by ignoring them), assuming that's its purpose.

From what I can tell, it is only possible for start == lowerCased.length() which is "safe" for subSequence(). That would happen for a string like "trailing,comma," which produces an extra empty string. But if we're concerned about that, then it seems ",leading,comma" and "inner,,comma" should also be disallowed.

While RFC 7230 §7 says:

In any production that uses the list construct, a sender MUST NOT
generate empty list elements.

It also says:

For compatibility with legacy list rules, a recipient MUST parse and
ignore a reasonable number of empty list elements

You may consider using StringUtil.unescapeCsvFields for this. It supports quotes, which is unnecessary here, but shouldn't hurt anything other than performance.

:( It seems StringUtil.unescapeCsvFields doesn't do the trimming, even though it is used by CombinedHttpHeaders.

mosesn · 2017-11-15T22:02:14Z

@ejona86 PTAL

ejona86

LGTM. Thank you. Sorry for being pedantic.

mosesn · 2017-11-16T15:28:40Z

No worries, I'll squash it, thanks for the reviews.

Motivation: Netty could handle "connection" or "te" headers more gently when converting from http/1.1 to http/2 headers. Http/2 headers don't support single-hop headers, so when we convert from http/1.1 to http/2, we should drop all single-hop headers. This includes headers like "transfer-encoding" and "connection", but also the headers that "connection" points to, since "connection" can be used to designate other headers as single-hop headers. For the "te" header, we can more permissively convert it by just dropping non-conforming headers (ie non-"trailers" headers) which is what we do for all other headers when we convert. Modifications: Add a new blacklist to the http/1.1 to http/2 conversion, which is constructed from the values of the "connection" header, and stop throwing an exception when a "te" header is passed with a non-"trailers" value. Instead, drop all values except for "trailers". Add unit tests for "connection" and "te" headers when converting from http/1.1 to http/2. Result: This will improve the h2c upgrade request, and also conversions from http/1.1 to http/2. This will simplify implementing spec-compliant http/2 servers that want to share code between their http/1.1 and http/2 implementations. [Fixes netty#7355]

normanmaurer · 2017-11-17T08:09:26Z

Let me pull this in now.. @Scottmitch feel free to open a follow-up PR if you want any changes and had time to review

normanmaurer · 2017-11-17T08:10:57Z

Cherry-picked into 4.1 (d976dc1)

normanmaurer · 2017-11-17T08:11:58Z

@mosesn thanks!

imasahiro · 2017-11-17T15:23:00Z

codec-http2/src/main/java/io/netty/handler/codec/http2/HttpConversionUtil.java

    public static void toHttp2Headers(HttpHeaders inHeaders, Http2Headers out) {
        Iterator<Entry<CharSequence, CharSequence>> iter = inHeaders.iteratorCharSequence();
+            new CharSequenceMap<AsciiString>();


Can we remove this allocation?

@imasahiro if you think we can easily please submit a PR :) Happy to review and merge

Isn't this a trash line? As in it does nothing? It doesn't seem like the object is used.

lol doh! ... I totally missed it. Removed in 78522cf

@normanmaurer Ah, you already pushed the comment! Thanks anyways :-)

oh whoops, sorry!

Scottmitch · 2017-11-20T17:18:38Z

thanks @mosesn ! generally lgtm .. few followup PRs coming up:

#7421
#7422

mosesn · 2017-11-20T18:18:28Z

Thanks @Scottmitch!

mosesn force-pushed the mosesn/strip-bad-headers branch from f3f1347 to dac6ee9 Compare November 10, 2017 22:13

mosesn mentioned this pull request Nov 10, 2017

HttpConversionUtil.toHttp2Headers should strip http/2 incompatible headers #7355

Closed

normanmaurer requested changes Nov 10, 2017

View reviewed changes

normanmaurer self-assigned this Nov 10, 2017

normanmaurer requested review from ejona86, Scottmitch and carl-mastrangelo November 10, 2017 23:15

ejona86 requested changes Nov 11, 2017

View reviewed changes

mosesn force-pushed the mosesn/strip-bad-headers branch from 9246b77 to 4b41e52 Compare November 13, 2017 19:16

normanmaurer approved these changes Nov 15, 2017

View reviewed changes

ejona86 requested changes Nov 15, 2017

View reviewed changes

ejona86 approved these changes Nov 15, 2017

View reviewed changes

ejona86 approved these changes Nov 16, 2017

View reviewed changes

mosesn force-pushed the mosesn/strip-bad-headers branch from babf555 to 2831e52 Compare November 16, 2017 15:29

normanmaurer added the defect label Nov 17, 2017

normanmaurer added this to the 4.1.18.Final milestone Nov 17, 2017

normanmaurer closed this Nov 17, 2017

imasahiro reviewed Nov 17, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

codec-http2: Improve h1 to h2 header conversion #7399

codec-http2: Improve h1 to h2 header conversion #7399

mosesn commented Nov 10, 2017

normanmaurer Nov 10, 2017

normanmaurer Nov 10, 2017

ejona86 Nov 11, 2017

normanmaurer Nov 13, 2017

mosesn Nov 13, 2017

ejona86 Nov 11, 2017

mosesn commented Nov 13, 2017

ejona86 Nov 15, 2017

ejona86 Nov 15, 2017

ejona86 Nov 15, 2017

mosesn commented Nov 15, 2017

ejona86 left a comment

mosesn commented Nov 16, 2017

normanmaurer commented Nov 17, 2017

normanmaurer commented Nov 17, 2017

normanmaurer commented Nov 17, 2017

imasahiro Nov 17, 2017

normanmaurer Nov 17, 2017

ejona86 Nov 17, 2017

normanmaurer Nov 17, 2017

imasahiro Nov 17, 2017

mosesn Nov 17, 2017

Scottmitch commented Nov 20, 2017 •

edited

mosesn commented Nov 20, 2017

codec-http2: Improve h1 to h2 header conversion #7399

codec-http2: Improve h1 to h2 header conversion #7399

Conversation

mosesn commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosesn commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosesn commented Nov 15, 2017

ejona86 left a comment

Choose a reason for hiding this comment

mosesn commented Nov 16, 2017

normanmaurer commented Nov 17, 2017

normanmaurer commented Nov 17, 2017

normanmaurer commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scottmitch commented Nov 20, 2017 • edited

mosesn commented Nov 20, 2017

Scottmitch commented Nov 20, 2017 •

edited