Repository: apple/swift-nio-http2 (swift)

Issue:
HTTP2ChannelHandler.wroteFrame is never returned to false
HTTP2ChannelHandler has a [flag](https://github.com/apple/swift-nio-http2/blob/cee48fd87d0ae541cefeee4880d2e419631e9368/Sources/NIOHTTP2/HTTP2ChannelHandler.swift#L64) indicating whether we wrote a frame. We check this before issuing a flush to avoid unnecessary flushes. However, we never return this value to `false` after issuing a flush [here](https://github.com/apple/swift-nio-http2/blob/master/Sources/NIOHTTP2/HTTP2ChannelHandler.swift#L582-L584). As a result we may emit flushes unnecessarily.

Pull request:
Motivation:

The `wroteFrame` flag in `HTTP2ChannelHandler` indicates whether a frame
has been written. This flag is checked before flushing in order to avoid
unnecessary flushes. Unfortunately this is only ever set to `true`.

Modifications:

- Add a `flushIfNecessary` to check and toggle `wroteFrame` and flush if
  necessary
- Add a test

Result:

- Few unnecessary flushes
- Resolves https://github.com/apple/swift-nio-http2/issues/245

Patch:
diff --git a/Sources/NIOHTTP2/HTTP2ChannelHandler.swift b/Sources/NIOHTTP2/HTTP2ChannelHandler.swift
index d263ee5b..7649ba2c 100644
--- a/Sources/NIOHTTP2/HTTP2ChannelHandler.swift
+++ b/Sources/NIOHTTP2/HTTP2ChannelHandler.swift
@@ -330,7 +330,7 @@ extension NIOHTTP2Handler {
         // The user may choose to fire a more specific error if they wish.
         let goAwayFrame = HTTP2Frame(streamID: .rootStream, payload: .goAway(lastStreamID: .maxID, errorCode: reason, opaqueData: nil))
         self.writeUnbufferedFrame(context: context, frame: goAwayFrame)
-        context.flush()
+        self.flushIfNecessary(context: context)
         context.fireErrorCaught(underlyingError)
     }
 
@@ -341,7 +341,7 @@ extension NIOHTTP2Handler {
         // the user's issue.
         let rstStreamFrame = HTTP2Frame(streamID: streamID, payload: .rstStream(reason))
         self.writeBufferedFrame(context: context, frame: rstStreamFrame, promise: nil)
-        context.flush()
+        self.flushIfNecessary(context: context)
         context.fireErrorCaught(underlyingError)
     }
 
@@ -379,7 +379,7 @@ extension NIOHTTP2Handler {
 
         let initialSettingsFrame = HTTP2Frame(streamID: .rootStream, payload: .settings(.settings(self.initialSettings)))
         self.writeUnbufferedFrame(context: context, frame: initialSettingsFrame)
-        context.flush()
+        self.flushIfNecessary(context: context)
     }
 
     /// Write a frame that is allowed to be buffered (that is, that participates in the outbound frame buffer).
@@ -591,8 +591,13 @@ extension NIOHTTP2Handler {
         }
         
         self.isUnbufferingAndFlushingAutomaticFrames = false
+        self.flushIfNecessary(context: context)
+    }
 
+    /// Emits a flush if a frame has been written.
+    private func flushIfNecessary(context: ChannelHandlerContext) {
         if self.wroteFrame {
+            self.wroteFrame = false
             context.flush()
         }
     }


Test patch:
diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests+XCTest.swift b/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests+XCTest.swift
index 86c56043..6dbbb530 100644
--- a/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests+XCTest.swift
+++ b/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests+XCTest.swift
@@ -75,6 +75,7 @@ extension SimpleClientServerFramePayloadStreamTests {
                 ("testGreasedSettingsAreTolerated", testGreasedSettingsAreTolerated),
                 ("testStreamCreationOrder", testStreamCreationOrder),
                 ("testStreamClosedInvalidRequestHeaders", testStreamClosedInvalidRequestHeaders),
+                ("testHTTP2HandlerDoesNotFlushExcessively", testHTTP2HandlerDoesNotFlushExcessively),
            ]
    }
 }
diff --git a/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift b/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift
index ed9bb2f9..55d4ab70 100644
--- a/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift
+++ b/Tests/NIOHTTP2Tests/SimpleClientServerFramePayloadStreamTests.swift
@@ -1960,4 +1960,26 @@ class SimpleClientServerFramePayloadStreamTests: XCTestCase {
         XCTAssertNoThrow(try self.clientChannel.finish())
         XCTAssertNoThrow(try self.serverChannel.finish())
     }
+
+    func testHTTP2HandlerDoesNotFlushExcessively() throws {
+        try self.basicHTTP2Connection()
+
+        class FlushCounter: ChannelOutboundHandler {
+            typealias OutboundIn = Never
+            var flushCount = 0
+
+            func flush(context: ChannelHandlerContext) {
+                self.flushCount += 1
+                context.flush()
+            }
+        }
+
+        let counter = FlushCounter()
+        XCTAssertNoThrow(try self.clientChannel.pipeline.syncOperations.addHandler(counter, position: .first))
+
+        // 'channelReadComplete' should cause the HTTP2Handler to emit a flush if any frames have
+        // been written. No frames have been written, so no flush should be emitted.
+        self.clientChannel.pipeline.fireChannelReadComplete()
+        XCTAssertEqual(counter.flushCount, 0)
+    }
 }