netty: Support pseudo-headers in more of the Http2Headers methods

[njhill]

This adds support for using pseudo-headers with get(), getAll(), remove() which indirectly also adds support for contains(), set() and setLong().

Also includes some other general simplifications/deduplication in GrpcHttp2HeadersUtils.

Fixes #8981

[njhill]

If there's interest in this I can add some tests.

[ejona86]

I'm annoyed with the unrelated changes in this PR. What was so wrong with size += super.size() that it needed to be changed?

I'd probably merge this even if it grinds my gears some. Except that getAll() returning emptyList() does concern me. I agree we should be able to make that change, but it's not something I want to mix in with a Netty compat fix.

I've gone and implemented this in #9004, and it was good I implemented basic tests. remove(), for example, was broken for :status.

[ejona86]

I'm not all that wild with unrelated performance optimizations being mixed in with compatibility fixes. It adds noise that makes it slower to review and it can add bugs/surprises. It seems half of this PR is unrelated stuff or maybe it is important, but I can't tell, which is bad in its own right. For the performance enhancement in getAll(), might there be some code that modifies the returned list ("for performance")? DefaultHeaders in Netty returns a mutable List, so this PR could create new incompatibilities while fixing old ones.

It seems this equals should just be str0.equals(str1). If someone is concerned about inline depth, then "don't use/delete the convenience method." We shouldn't care much about the performance of toString(), certainly not enough to delay a more urgent PR.

[ejona86]

I don't know. It seems it'd be easier to keep addPseudoHeader() as it was and just have a removePseudoHeader() method. It'd be similar complexity to getPseudoHeader().

If I'm concerned with the amount of duplication, I think just having primitive set and get (without the "is already set" check) methods for pseudo headers gets you furthest. add() would have if (getPseudoHeader(name) != null) throw X; setPseudoHeader(name, value);.

[njhill]

Thanks @ejona86, apologies for the unrelated changes. It's a bad habit that I find myself making simplifications while restructuring, you're right that as mentioned in the PR description the resulting changes include some performance and general structural simplification changes in addition to the new function.

getAll() returning emptyList() does concern me

The javadoc states that the returned list can't be modified and that it will be an empty list if no values are found. Fair enough though if this is still a concern compatibility-wise.

Incidentally there was also a small (probably unimportant) bug fixed where an absent transfer encoding header would return a singleton containing null.

It seems this equals should just be str0.equals(str1).

That seems reasonable, I wasn't sure really why this particular convenience method was there in the first place. I added the hashcode check because this is what the AsciiString#equals impl does anyhow.

We shouldn't care much about the performance of toString(), certainly not enough to delay a more urgent PR.

This was purely for simplification, I wasn't considering performance.

What was so wrong with size += super.size() that it needed to be changed?

I guess it's just my OCD, to me this kind of thing is just unnecessary verbosity

size += super.size();

return size;

As mentioned I was planning to add tests but thought I would check that there was interest first.

[ejona86]

I don't think this entire PR needs to be trashed. Improving equals() to use hashcode (although probably via AsciiString.equals()) and the getAll(TE_HEADER) fixes are both great.

The getAll() allocation optimization is probably "meh", since it isn't a hot path. The optimization isn't too ugly, but the gain is essentially 0. I do think we can make the change, although I may want to do a quick audit that nothing obvious is modifying the list.

I'm definitely a fan of sprinkling more finals in an abstract class...

Although I pretty much despise the CharSequence → AsciiString pre-conversion helpers in the base class. That stuff is the bane of my existence when reading DefaultHeaders in Netty. Far, far too many similar methods in Headers. The cost of indirection (for a human) feels too high for the small reduction in code duplication. To be clear though, I'd allow this sort of thing in new code or an author changing their original code. Me disliking it is just "I don't see as much value in the change for its own sake." We need a concrete reason for the change instead of preference.

The toString() change is "fine", but probably not worth it, since the legibility is virtually the same (some improvements, some decreases). So again, I'm giving preference to the existing code. I will say I'm particularly sensitive to a base class calling protected methods on this, so you may get a bit different result for the "how good of a change is this" calculus. Although I probably would have written it more like you have in this PR; I'm not just defending my own preferences.

Incidentally there was also a small (probably unimportant) bug fixed where an absent transfer encoding header would return a singleton containing null.

Oh! I had that same bug in my code, which the tests ended up catching.

I wasn't sure really why this particular convenience method was there in the first place. I added the hashcode check because this is what the AsciiString#equals impl does anyhow.

Yeah, I was initially like, "why does this exist." But then looking around, I feel confident it was because there's lots of code that will do various combinations of equals with AsciiString and byte[] and it just gets obnoxious to figure out the "appropriate" approach for each.

I thought you might have avoided AsciiString.equals() because of inline depth. I didn't think about how confusion of "why does this exist" would have made you more cautious. Makes sense.

I guess it's just my OCD, to me this kind of thing is just unnecessary verbosity

Yeah, this gets into personal styles. Depending on circumstance I might do it either of the two ways, and likely be annoyed by the alternative. But I learn to live with it, otherwise I change it and then the original author changes it back and now we are in a formatting war. I really resist the urge with the empty line after class Foo { that some people are consistent about. And checkNotNull(). And... But it only is appropriate to preserve the existing style when modifying a file.

I try hard not to make random unnecessary edits like that, but I know others don't feel the same and are okay with a few small instances. If it was done by an automated tool, I have people revert it, because formatting wars are bad with IDE formatters. But otherwise I generally let some amount of it through. I was only especially hard on this PR because it was fixing a semi-urgent compat issue that we also may want to backport, so it needs to skew toward conservative changes.

As mentioned I was planning to add tests but thought I would check that there was interest first.

As I said, I would have actually taken it as-is. I was just pointing out that the tests turned out to be fruitful. I think it was appropriate to wait on the tests.

[njhill]

Thanks @ejona86

Oh! I had that same bug in my code, which the tests ended up catching.

I'm actually not sure what you're referring to w.r.t. the :status bug, but here I was referring to the getAll(TE_HEADER) bug which I also commented isn't fixed in your PR. Maybe dedicated PR for that?

Although I pretty much despise the CharSequence → AsciiString pre-conversion helpers in the base class. That stuff is the bane of my existence when reading DefaultHeaders in Netty. Far, far too many similar methods in Headers. The cost of indirection (for a human) feels too high for the small reduction in code duplication. To be clear though, I'd allow this sort of thing in new code or an author changing their original code. Me disliking it is just "I don't see as much value in the change for its own sake." We need a concrete reason for the change instead of preference.

The main thing I was addressing here was the inconsistency in how differences between GrpcHttp2RequestHeaders and GrpcHttp2ResponseHeaders method impls are currently handled (the only subclasses of abstract GrpcHttp2InboundHeaders). For example why is public getAll defined in the base class and overridden just in GrpcHttp2RequestHeaders but public get and add not defined in the base class and instead defined separately in both subclasses? It seemed much cleaner to just move the remaining GrpcHttp2ResponseHeaders methods into the base class, especially since GrpcHttp2RequestHeaders is in a sense a specialization of that (augmenting the pseudoheaders). Then GrpcHttp2ResponseHeaders becomes empty.

I try hard not to make random unnecessary edits like that

I agree, I guess in this case it looked more like an omission than style difference and so was in the spirit of the other general streamlining.