The CONTRIBUTING.md states that ruby generate_linux_tests.rb should used to generate the tests for Linux in the root directory. The file isn't in the root directory

Using NIOHTTP2 version 1.0.1.

Caveat: We might be doing something wrong in SwiftGRPC and/or this might be fixed by @Lukasa's recent changes, but I figured you guys might want to take a look.

When I run https://github.com/grpc/grpc-swift/blob/da1a59c919555367abfd6e2ffb285a175ffa11c0/Tests/SwiftGRPCNIOTests/NIOFunctionalTests.swift#L79 in Release mode with numberOfRequests = 2_000:

Test Suite 'Selected tests' started at 2019-04-08 14:11:22.901
Test Suite 'SwiftGRPCNIOTests.xctest' started at 2019-04-08 14:11:22.901
Test Suite 'NIOFunctionalTestsInsecureTransport' started at 2019-04-08 14:11:22.901
Test Case '-[SwiftGRPCNIOTests.NIOFunctionalTestsInsecureTransport testUnaryLotsOfRequests]' started.
1000 requests sent so far, elapsed time: 0.32004
total time to send 2000 requests: 0.852957
total time to receive 2000 responses: 2.479385
Test Case '-[SwiftGRPCNIOTests.NIOFunctionalTestsInsecureTransport testUnaryLotsOfRequests]' passed (1.644 seconds).
Test Suite 'NIOFunctionalTestsInsecureTransport' passed at 2019-04-08 14:11:24.545.
	 Executed 1 test, with 0 failures (0 unexpected) in 1.644 (1.644) seconds
Test Suite 'SwiftGRPCNIOTests.xctest' passed at 2019-04-08 14:11:24.545.
	 Executed 1 test, with 0 failures (0 unexpected) in 1.644 (1.644) seconds
Test Suite 'Selected tests' passed at 2019-04-08 14:11:24.546.
	 Executed 1 test, with 0 failures (0 unexpected) in 1.644 (1.645) seconds
Program ended with exit code: 0

When run with numberOfRequests = 4_000:

Test Suite 'Selected tests' started at 2019-04-08 14:11:36.312
Test Suite 'SwiftGRPCNIOTests.xctest' started at 2019-04-08 14:11:36.312
Test Suite 'NIOFunctionalTestsInsecureTransport' started at 2019-04-08 14:11:36.312
Test Case '-[SwiftGRPCNIOTests.NIOFunctionalTestsInsecureTransport testUnaryLotsOfRequests]' started.
1000 requests sent so far, elapsed time: 0.345793
2000 requests sent so far, elapsed time: 0.896414
3000 requests sent so far, elapsed time: 2.113964
total time to send 4000 requests: 3.534887
total time to receive 4000 responses: 7.418182
Test Case '-[SwiftGRPCNIOTests.NIOFunctionalTestsInsecureTransport testUnaryLotsOfRequests]' passed (4.771 seconds).
Test Suite 'NIOFunctionalTestsInsecureTransport' passed at 2019-04-08 14:11:41.084.
	 Executed 1 test, with 0 failures (0 unexpected) in 4.771 (4.772) seconds
Test Suite 'SwiftGRPCNIOTests.xctest' passed at 2019-04-08 14:11:41.085.
	 Executed 1 test, with 0 failures (0 unexpected) in 4.771 (4.772) seconds
Test Suite 'Selected tests' passed at 2019-04-08 14:11:41.085.
	 Executed 1 test, with 0 failures (0 unexpected) in 4.771 (4.773) seconds
Program ended with exit code: 0

Partial Instruments screenshots:

we should have API docs for this at https://docs.swiftnio.io

HTTP2StreamChannel drops the pending reads by allocating a new CircularBuffer. There's no need for that, just use removeAll: it's faster.

Given that we require users to use stuff from NIOHPACK, we should probably make it a product so that SwiftPM packages can depend on it.

This SE I think will be breaking the test suite: https://forums.swift.org/t/accepted-se-0274-concise-magic-file-names/34115

The

do {
    try someOperation()
XCTFail("should throw") // easy to forget
} catch error as SomethingError {
    XCTAssertEqual(.something, error as? SomethingError)
} catch {
    XCTFail("wrong error")
}

pattern is not only very long, it's also very error prone. If you forget
any of the XCTFails, you might not tests what it looks like

XCTAssertThrowsError(try someOperation) { error in
    XCTAssertEqual(.something, error as? SomethingError)
}

is much safer and shorter.

We should replace all uses of this pattern in Tests/**.

See also apple/swift-nio#1430 which does the same for swift-nio.

The RFC seems to be unclear here but the gist of the issue is that https://nghttp2.org/ when receiving a request on stream one replies with a PUSH_PROMISE (on stream 1, for stream 2) before having sent any HEADERS frames.

That leads to NIOHTTP2.HTTP2StreamStateMachine being in state halfClosedLocalPeerIdle when receiving the PP to which it responds with .init(result: .streamError(streamID: self.streamID, underlyingError: NIOHTTP2Errors.BadStreamStateTransition(), type: .protocolError), effect: nil).

Given the high profile of nghttp2, we will probably need to accept this. Screenshot shots the H2 protocol chat, 1.2.3.4:12345 being the remote peer, the 17.x.x.x being the local peer which is the client).

tested with swift-nio-http2 commit 492f375 and the http2-client from swift-nio-examples. Note, these might be bugs in http2-client too, I know it doesn't race the connections properly which might be the source of the hangs.

• ERROR: InvalidStreamIDForPeer():
  • https://amazon.com (ALPN fails, http2-client doesn't check)
  • https://vimeo.com (ALPN fails, http2-client doesn't check)
  • https://bit.ly (ALPN fails, http2-client doesn't check)
  • https://baidu.com (ALPN fails, http2-client doesn't check)
  • https://github.com (ALPN fails, http2-client doesn't check)
  • https://soundcloud.com (ALPN fails, http2-client doesn't check)
  • https://paypal.com (ALPN fails, http2-client doesn't check)
  • https://imdb.com (ALPN fails, http2-client doesn't check)
• ERROR: uncleanShutdown:
  • https://microsoft.com/ (Regular TLS error, http2-client should suppress)
  • https://weebly.com (Regular TLS error, http2-client should suppress)
• hangs:
  • https://godaddy.com (ALPN fails, http2-client doesn't check)
  • https://yahoo.com
  • https://qq.com
  • https://huffingtonpost.com (ALPN fails, http2-client doesn't check)
• ERROR: handshakeFailed(NIOSSL.BoringSSLError.sslError([Error: 268436576 error:10000460:SSL routines:OPENSSL_internal:reason(1120)])) (unlikely to be h2 related)
  • https://ebay.com (ALPN fails and eBay, unlike basically all other implementations, sends back a fatal error instead of completing the handshake)

curl https://moz.com/top500/domains/csv  | cut -d, -f2 | tr -d '"' | while read site; do if curl --http2-prior-knowledge -s "https://$site" > /dev/null; then echo $site; ./.build/x86_64-apple-macosx/debug/http2-client "https://$site" > /dev/null || echo "BAD $site"; fi; done

Zero length data frames aren't parsed correctly in HTTP2FrameParser.swift:

In #214 we deprecated and replaced the old stream channel. When we do a 2.0.0 release, we should remove the old code paths, reducing binary size, compile time, and resolving some minor performance problems.

• Remove the deprecated multiplexers and all tests that reference them.
• Remove the genericism from HTTP2StreamChannel, as it’s no longer necessary.
• Remove the extra code paths from MultiplexerAbstractChannel: it can concretely hold a single HTTP2StreamChannel
• Remove the initializers and code paths in HTTP2StreamMultiplexer that are deprecated or no-longer called
• Remove all deprecated pipeline helpers.

When closing a child channel that was locally constructed, if that channel has never had a frame sent on it then the close will never complete. This is because the attempt to send a RST_STREAM frame will fail.

We should keep track of whether we have ever received a stream created event, because if we haven't we can insta-close the channel.

While I was debugging #104, I spotted that we have an unnecessary heap allocation in ByteBuffer.readFrameHeader:

This whole method should probably be rewritten with readWithUnsafeReadableBytes or by using some readInteger calls: we can benchmark the difference between those two implementations.

The example server at "Sources/NIOHTTP2Server/main.swift" can currently only be communicated with using a specific curl command line, not a web browser. In order for a community to begin experimenting with this project there needs to be an example that communicate with a web browser out of the box.

curl --http2-prior-knowledge

grpc-swift version 1.0.0-alpha.17, swift-nio-http2 from: "1.12.1"

grpc-swift client(iOS App) --- nginx --- grpc server

client sends 3 different rpcs almost at the same time, one failed due to StreamIDTooSmall, the other two failed due to unavailable. error log:

2020-07-27T15:19:49+0800 error: connection_id=4003FC27-24A7-4B11-B041-37FDCA5E35C1/0 error=StreamIDTooSmall() grpc client error
2020-07-27T15:19:49+0800 info: new_state=transientFailure old_state=ready connection_id=4003FC27-24A7-4B11-B041-37FDCA5E35C1/0 connectivity state change
"gRPC state did change from ready to transientFailure"
2020-07-27T15:19:49+0800 error: request_id=9168A18B-3ED8-43F2-BCF5-188C1F68FF9A call_state=active connection_id=4003FC27-24A7-4B11-B041-37FDCA5E35C1/0 error=unavailable (14) grpc client error
2020-07-27T15:19:49+0800 error: call_state=active request_id=517D243A-8A9B-413C-97E9-7078E3866614 connection_id=4003FC27-24A7-4B11-B041-37FDCA5E35C1/0 error=unavailable (14) grpc client error

But from nginx log, 2 out of 3 responses are returned successfully. one had error due to client prematurely closed connection while processing HTTP/2 connection which we assume is due to client side close and reconnect.

we suspect in createStreamChannel, self.nextOutboundStreamID = HTTP2StreamID(Int32(streamID) + 2) isn't thread safe, but we couldn't find any doc or guidance what to do.

A very common use-case is to have a "HTTP Server" that wants to be able to speak both HTTP/1 and HTTP/2. This server will use ALPN to negotiate which HTTP protocol it speaks as part of the TLS handshake, and then configure the pipeline dynamically using the ApplicationProtocolNegotiationHandler. To make users's lives easier, they will almost certainly use the HTTP2ToHTTP1ServerCodec to bridge the abstraction boundary, and then have a "common" HTTP pipeline that is inserted either into every HTTP/1 channel or to every HTTP/2 stream channel.

While this is a very common pattern, the ApplicationProtocolNegotiationHandler is written to solve a very general version of this problem. This means every implementer is forced to write a lot of code that is basically always identical:

1. Check the ALPN negotiation, defaulting nil to "http/1.1".
2. Configure the normal parts of the HTTP pipeline
3. Add their handlers as needed

This duplication is a bit sad, especially as there's only really one correct implementation.

We can help here by adding a Channel.configureMultiProtocolHTTPServer function. This function would handle the common parts of that setup: adding an ApplicationProtocolNegotiationHandler that can handle both "h2" and "http/1.1" protocols, that knows how to do the fallback, and that knows how to configure the HTTP pipelines for the basic use-cases.

In the HTTP/1.1 case, it'll call ChannelPipeline.configureHTTPServerPipeline, putting in all the appropriate handler types, and then calling the user-provided initializer. In the HTTP/2 case it'll call Channel.configureHTTP2ServerPipeline, and provide a stream channel initializer that inserts the HTTP2ToHTTP1ServerCodec handler and then calls the user-provided initializer.

The signature should be:

extension Channel {
    func configureMultiProtocolHTTPServer(_ channelInitializer: @escaping (Channel) -> EventLoopFuture<Void>) -> EventLoopFuture<Void>
}

This would be of real value to folks like @tanner0101 if they want to add HTTP/2 server support, as they should be able to abstract away essentially all of the boilerplate. A chunk of similar code exists in grpc-swift.

Currently, we have some HPACK performance tests expressed as XCTests. Unfortunately, those randomly fail if the build nodes are too busy.

We should use the normal NIO perf testing framework for those instead of XCTest and use XCTest purely as a unit/integration testing framework.

I've just branched nghttp2-support-branch, so we can begin moving forward for the NIO 2.0 release cycle by updating the CI on this repository.

@tomerd, can we change the CI setup so that the master branch has only got a Swift 5.0 builder (for both the branch and PRs), and the nghttp2-support-branch has a Swift 4.1 and Swift 4.2 builder?

The current stream flow control implementation eagerly credits the stream flow control window back to the remote peer. It shouldn't: it should credit that back to the peer only when the frame has been delivered to the stream channel pipeline.

@glbrntt is going to take this one

The code that handles shutting down on inboundConnectionError tends to hit InvalidStreamIDForPeer. This is because we attempt to send a GOAWAY with lastStreamID set to .maxID, which is technically an invalid stream ID for the peer. We should modify the check here, as it's reasonable to want to set .maxID in this case, even though the server cannot technically emit such a stream ID.

HTTP2StreamMultiplexer should forward channelActive through the root channel pipeline but doesn't.

release: 1.0.0

Even in Swift 5, enums defined outside of the standard library, C headers, and swift overlays will be considered frozen and immutable, meaning that a modification to one will be a semver-major version-breaking change. Our HTTP2Frame.FramePayload type is (still) an enum, and we literally cannot do anything about it without dropping in a C header enumerating the known frame ID values.

There are already two published RFCs that have added new frame types to HTTP2: the ALTSVC frame defined by RFC 7838 § 4 and the ORIGIN frame defined by RFC 8336 § 2. Those are included already (or are going to be included in my encoder/decoder patch), but more will doubtless arrive at some point.

Is there anything we can do about this? My original implementation used a Frame protocol and subtypes, with a switch over the type (i.e. case is DataFrame: or case let x as DataFrame:) to disambiguate them. Is this a better option, and if so, shouldn't we try to land this before swift-nio-http2 transitions off its nghttp2 dependency altogether?

• we should probably lower-case the header names automatically
• we should probably just ignore connection: keep-alive/close
• we should probably drop transfer-encoding: chunked

usually the configure* methods are on the ChannelPipeline. For some reason, configureHTTP2Pipeline is an extension on Channel rather than ChannelPipeline. Any particular reason or just an oversight?

CI should check API breakages

we need to link the docs from the README.

In swift-nio-http2 today we have an unspoken hard requirement, which is that if you call createStreamChannel multiple times you must send the first write on each of those channels in the order you created them. This is because createStreamChannel allocates a stream ID immediately for each of those channels, but that allocation isn't meaningful until we try to write to the network. If you write out of order, the later streams will use the higher stream ID allocated to them, and will implicitly retire the lower IDs used by the earlier channels. When those earlier channels try to send, the core state machine will reject them for violating stream ID ordering.

After discussing with @glbrntt we think this is made up of several changes:

• Define a new internal protocol: HTTP2FrameConvertible. This is a protocol that encapsulates creating HTTP2Frames from a given object and vice-versa. We will have two conforming types: HTTP2Frame (trivial) and HTTP2Frame.Payload (straightforward, but not trivial). (#216)
• Define a new protocol: HTTP2FramePayloadConvertible. This is the inverse of the previous protocol: creates a payload from the given object, or vice versa. (#216)
• Make HTTP2StreamChannel generic over an outbound payload type conforming to HTTP2FrameConvertible. This will become the inbound and outbound message type. This requires updates to the various inbound and outbound reading and writing functions. This should not require any interface changes on the interface between the multiplexer and the stream. The multiplexer will need to be updated to hold the new type (HTTP2StreamChannel<HTTP2Frame>). (#218)
• Define a struct that provides the interface HTTP2StreamMultiplexer expects from HTTP2StreamChannel, that holds the channel privately. This should be backed by an enum, which we will use later to add a new case for the new channel type. Refactor HTTP2StreamMultiplexer to use this type. (#215)
• Update HTTP2StreamChannel to tolerate its streamID being optional, and nil on initialisation. This requires a new function on HTTP2StreamMultiplexer to get the next stream ID, which is currently done in createStreamChannel. (#217)
• Add a new createStreamChannel function with a new initializer that no longer gets a stream ID. Plumb through this initializer to create new stream channels that use HTTP2Frame.Payload instead of HTTP2Frame. Update the enum created above for the new case. (#221)
• Update the codecs to become generic over HTTP2FramePayloadConvertible, and rename them. Create typealiases for both the old version (keeping its old name) and the new one that uses HTTP2FramePayload. Deprecate the old typealias. (#222)
• Deprecate the old interface, including replacing anything in the HTTP2PipelineHelpers function to use the new initializer. (#226, #227)

Example branch: https://github.com/mrmage/grpc-swift/tree/nio

Example output from my webserver (libnghttp2-dev is installed):

timing@forumd ~/grpc-swift $ time swift package resolve
Fetching https://github.com/apple/swift-protobuf.git
Fetching https://github.com/kylef/Commander.git
Fetching https://github.com/apple/swift-nio-zlib-support.git
Fetching https://github.com/apple/swift-nio.git
Fetching https://github.com/apple/swift-nio-http2.git
Fetching https://github.com/apple/swift-nio-nghttp2-support.git
Fetching https://github.com/apple/swift-nio-ssl-support.git
Fetching https://github.com/kylef/Spectre.git
Cloning https://github.com/apple/swift-nio.git
Resolving https://github.com/apple/swift-nio.git at 1.9.4
Cloning https://github.com/apple/swift-protobuf.git
Resolving https://github.com/apple/swift-protobuf.git at 1.1.1
Cloning https://github.com/kylef/Spectre.git
Resolving https://github.com/kylef/Spectre.git at 0.8.0
Cloning https://github.com/apple/swift-nio-zlib-support.git
Resolving https://github.com/apple/swift-nio-zlib-support.git at 1.0.0
Cloning https://github.com/apple/swift-nio-ssl-support.git
Resolving https://github.com/apple/swift-nio-ssl-support.git at 1.0.0
Cloning https://github.com/kylef/Commander.git
Resolving https://github.com/kylef/Commander.git at 0.8.0
Cloning https://github.com/apple/swift-nio-nghttp2-support.git
Resolving https://github.com/apple/swift-nio-nghttp2-support.git at 1.0.0
Cloning https://github.com/apple/swift-nio-http2.git
Resolving https://github.com/apple/swift-nio-http2.git at master
error: terminated(1): git -C /home/timing/grpc-swift/.build/checkouts/swift-nio-http2.git-423060702173962331 submodule update --init --recursive output:
    Submodule 'hpack-test-case' ([email protected]:http2jp/hpack-test-case) registered for path 'hpack-test-case'
    

real	0m12.375s
user	0m8.047s
sys	0m1.904s

On Travis CI, this also fails. On my local 10.13.6 machine with Xcode 10, swift package resolve works fine.

Any ideas? Would it maybe make a difference if the submodule URL in https://github.com/apple/swift-nio-http2/blob/master/.gitmodules was provided as an https:// URL instead?

We should ensure that this doesn't keep happening.

see apple/swift-nio#969

In v2, we should bring the HTTP2StreamMultiplexer into the NIOHTTP2Handler and integrate them together, rather than having them be separate channel handlers.

While it's been a nice design idea to separate these out, the result of that change has been that we incur an expensive and very noisy interface between these two tightly-related classes. The NIOHTTP2Handler has to tell the HTTP2StreamMultiplexer some metadata on almost every frame (flow control changes, which affect the sending and receiving of DATA and WINDOW_UPDATE frames, and stream state changes, which affect HEADERS, RST_STREAM, and GOAWAY: basically all the frames, then). This metadata requires an extra pipeline walk to communicate, leading to a load of needless ARC traffic and extra cost.

Rather than incur that cost, we should smush these two together and take advantage of the fact that they are, in fact, tightly coupled. This should give us a nice performance win as well as a huge correctness boost.

Currently, the error look like this:

NIOHTTP2Errors.NoSuchStream(...)

but I think it should be either

NIOHTTP2Error.noSuchStream(...) // imitating an open enum on top of a struct

or

NIOHTTP2Errors.NoSuchStreamError(...)

IMHO, preferably the former

When we receive a DATA frame with END_STREAM set that terminates a stream, we don't emit the correct events to cause the HTTP2StreamMultiplexer to emit WINDOW_UPDATE frames for the connection. In the worst possible cases this could actually lead to deadlock if we allow the connection window to go to zero here.

NIOHTTP2 relies on a number of HTTP/2 user events that are passed down the channel pipeline. These are necessarily stuffed into existential containers for the Any type, and so need to fit into 3 words or fewer to avoid a heap allocation.

As these user events exist to pass data through the channel pipeline, they will always be stuffed into an existential after their construction. That means if any of these are wider than 3 words or use an excessively large number of refcounted objects, they should probably become class-backed-structs to reduce the cost of passing them around. Turns out, NIOHTTP2WindowUpdatedEvent is 33 bytes wide, which makes it too big. Let's shrink this!

While it stores a pair of Int?, it forcibly constrains them in the initializer to fit into the Int32 range. This already basically doesn't work as they can subsequently be edited, so we should probably update this to store some private Int32s and just use computed properties to access the values as Int.

close()-ing a newly created stream channel causes a fatal error because a nil optional (networkStreamID) is forcibly unwrapped here.

The documentation for networkStreamID says it will be nil if "the stream has not yet reached the network".

Closing a stream prior to it reaching the network seems like valid behaviour, albeit an edge case, which shouldn't cause a fatal error.

In validateSettings() (ConnectionStateMachine.swift:1311)

            case .maxFrameSize:
                guard setting._value >= (1 << 14) && setting._value < ((1 << 24) - 1) else {

However, (1 << 24) - 1 is a valid frame size. The comparison should be setting._value < (1 << 24)

I'm just beginning to dig into the HTTP/2 and Swift-NIO code and wanted to get some feedback on a proposal to modify BadStreamStateTransition to have from/to states to improve debuggability when it gets logged (and maybe in the future, stack traces when it gets created)

A few questions:

• The HTTP2StateMachine.State enum is private, and comments indicate that it's so people don't erroneously set the state directly. Isn't the right place to do this in setting the member state variable to private (which it already is)? If that works I could make it internal and move the state enum outside of HTTP2StateMachine

• I'm seeing some comments that (to me) don't agree with the RFC. I am no HTTP expert by any means and I'm fairly new to Swift, as well, so I wouldn't be surprised if I"m wrong here, but on line 379, it indicates sending HEADERS on an idle stream isn't permitted, but the state transition diagram and the RFC both seem to indicate that it should transition a stream to open.

Apologies for the basic questions, like I mentioned I'm new. Thanks in advance for your help.

Neal

DataBuffer.evacuatePendingWrites creates an "empty" MarkedCircularBuffer and swaps it with the existing one. This is fine, but it forces an allocation. It would be better to have a singleton empty MCB and store that into the variable to avoid the extra allocation. We'll still have to allocate if we actually write into it, but that's fine, as we know we never will.

It seems that there are some issues in autoRead implementation, they don't fire down the pipeline

RFC 8441 defines "extended CONNECT", which is primarily useful for implementing websockets-over-HTTP/2. We right now do not support this, but we'd like to. This shouldn't be too much work.

If a stream has had all its data frames emitted, such that the HTTP2StreamDataProvider is in the .pending state, and the user then submits a trailer frame, that frame will never be emitted. This is because we don't correctly resume the data provider in this case.

According to RFC 7540 (§ 8.1.2.2):

An endpoint MUST NOT generate an HTTP/2 message containing connection-specific header fields; any message containing connection-specific header fields MUST be treated as malformed

Currently, NIOHTTP2 allows fields like Connection to be serialized. These fields should either be:

1. Stripped from the response.
2. Result in a serialization error.

I personally prefer (2) slightly since I'd rather remove these headers myself if I have to than be wasting CPU time adding them only to have them silently removed.

This is a collections of things that need an allocation counter test to make sure we don't regress again. This isn't done yet because we haven't productised the alloc counter tests yet.

• the CoW fixed in #104
• the heap allocation listed in #105 (obviously to be fixed first)

creating a child channel in the childChannelInitializer

        func requestStreamInitializer(channel: Channel, streamID: HTTP2StreamID) -> EventLoopFuture<Void> {
            return channel.pipeline.addHandlers([HTTP2ToHTTP1ClientCodec(streamID: streamID, httpProtocol: .https),
                                                 SendAGETRequestHandler(responseReceivedPromise: self.responseReceivedPromise)],
                                                first: false)
        }

        self.multiplexer.createStreamChannel(promise: nil, requestStreamInitializer)

will have SendAGETRequestHandler see channelActive when ctx.channel.parent!.isActive is still false. That's a problem because when sending a request in the child's channelActive the program will crash as the HTTP2Parser's session: NGHTTP2Session! is still nil, ie. we'll crash.

CI should enable TSan

Current situation

• Swift 4.0 is not supported for swift-nio-http2 (neither 0.1.0 nor master compiles)
• Package.swift claims Swift 4.0 support (via // swift-tools-version:4.0)

Possible fixes:

• officially don't support Swift 4.0
• fix 4.0 compilation.

paging @normanmaurer , @AlanQuatermain , @Lukasa , @ianpartridge, @pushkarnk, @tanner0101, @0xTim

When the HPACKEncoder was first written it was written to provide incremental header compression. This was enhanced later on in the development of v1 to provide one-shot header compression, which is what NIOHTTP2 uses today.

The presence of the incremental compression mode with beginEncoding/endEncoding forces some awkward compromises in the one-shot compression mode, which means that beginEncoding/endEncoding are forced to do weird extra allocations for no particularly good reason. Of course, this doesn't matter really, because we don't use the functions anyway, but they're part of the public interface to NIOHPACK and so cannot be changed right now.

In v2 we should just delete those functions. They're not useful, and cause more complexity than necessary.

We tried to fix this with #233, but a case was missed. If we receive a number of DATA frames in one channelRead cycle, one of which would trigger a window update frame but another, later one of which would set END_STREAM, we will incorrectly still emit window update frames because we won't spot that END_STREAM in time. We need to move that check.

needs -warnings-as-errors