transport: fix handling of header metadata in serverHandler

[misberner]

This PR changes the serverHandlerTransport code to take header metadata from the stream (set via invocations of SetHeader) into account, both when implicitly sending headers, or when explicitly sending headers via SendHeader (merging any previously set metadata with the additionally supplied one, as per the docs).

It also fixes an issue with SendHeader where modifications to the passed metadata map after the SendHeader call has already returned might still affect the header values that get written to the stream.

Please check out the first commit in this branch (3140f261) for a state that only has the modified test (which of course fails) added.

Fixes #3483.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Malte Isberner (3140f26, 5675b98)

[misberner]

tbh I don't really understand why WriteHeader even takes an md parameter - IMHO it would be more elegant to implement (*Stream).SendHeader via

s.SetHeader(md)
s.st.WriteHeader(s)

Since ServerTransport is an internal interface, I don't see any issues, but it would require changing the http2Server implementation as well, which is why I opted for a more local change.

[misberner]

CI errors should now be fixed as well.

[dfawley]

It also fixes an issue with SendHeader where modifications to the passed metadata map after the SendHeader call has already returned might still affect the header values that get written to the stream.

Can you explain this a little more? What might be modifying the metadata map and why? I don't believe this should be happening as the metadata godoc says modifying metadata stored in contexts is not allowed (e.g.).

[dfawley]

Actually, nevermind my last question; I see the problem. I think there's a cleaner design here somewhere, but your changes LGTM.

[misberner]

Can you explain this a little more? What might be modifying the metadata map and why? I don't believe this should be happening as the metadata godoc says modifying metadata stored in contexts is not allowed (e.g.).

This was a concurrency error/race condition. SendHeader returns once ht.do(func() { ... }) returns, which however only means that the closure has been pushed to the channel/accepted by the receiver, but not necessarily finished dispatching by the receiver. For this reason, a call

md := metadata.Pairs("foo", "bar")
s.SendHeader(md)
delete(md, "foo")

might non-deterministically result in a header with the "foo" key being written or not, since the dispatching of the closure which reads md happens concurrently with delete(md, "foo") without synchronization. That's why I changed the implementation of SendHeader to call SetHeader first, which ensures that no references to the passed MD map are retained after SendHeader returns.