Make Http2UnknownFrame extend Http2StreamFrame rather than Http2Frame

[Bennett-Lynch]

This is a follow-up issue regarding: #7860

Expected behavior

Writing an Http2UnknownFrame to a child channel will be written to the parent channel and underlying HTTP/2 connection via Http2MultiplexCodec.

When Http2MultiplexCodec reads an Http2UnknownFrame, it will be propagated to the correct child channel.

Actual behavior

Http2MultiplexCodec rejects the write since Http2UnknownFrame is not an instanceof Http2StreamFrame.

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

Lines 1021 to 1028 in dfeb4b1

	} else {
	String msgStr = msg.toString();
	ReferenceCountUtil.release(msg);
	promise.setFailure(new IllegalArgumentException(
	"Message must be an " + StringUtil.simpleClassName(Http2StreamFrame.class) +
	": " + msgStr));
	return;
	}

Http2MultiplexCodec does not propagate reads to child channels since Http2UnknownFrame is not an instanceof Http2StreamFrame.

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

Lines 219 to 221 in dfeb4b1

	if (frame instanceof Http2StreamFrame) {
	Http2StreamFrame streamFrame = (Http2StreamFrame) frame;
	onHttp2StreamFrame(((Http2MultiplexCodecStream) streamFrame.stream()).channel, streamFrame);

Proposal

Http2UnknownFrame should extend Http2StreamFrame, rather than Http2Frame, as it already defines the two stream-related methods that the two interfaces differ in.

Should it possibly declare an isEndStream method as well, similar to Http2DataFrame?

[normanmaurer]

@Bennett-Lynch let it extend Http2StreamFrame sounds like the correct thing to do. Would you be interested in proving a PR ?

I think I would leave the isEndStream as it is and just use the Http2Flags for it.

[normanmaurer]

@Bennett-Lynch I had a bit time this morning so I just did it myself and adding unit tests:

#7976

PTAL

[Bennett-Lynch]

Thanks, @normanmaurer -- change looks good to me.

Fine with using Http2Flags for detecting isEndStream, but may I ask your reasoning for it? Maintaining a consistent interface across frame types seems more ideal, but I realize these UnknownFrames are a bit of an edge case.

[normanmaurer]

@Bennett-Lynch because its redundant as it exists in the flags already (and you can have other flags as well).