Mitigate zero-length writes issue.

Motivation:

On some iOS releases, zero-length writes cause problems for
Network.framework. These problems can lead to difficult to debug
catastrophic failure modes, so they ought to be avoided.

NIOTransportServices provides a workaround channel handler, which we
ought to use.

Modifications:

- Add a PlatformSupport check for whether to apply the workaround.
- Add the workaround code to the pipeline configuration callbacks.
- Tests

Results:

Users can work around the zero length writes problem.

Author: Lukasa

Sources/GRPC/ClientConnection.swift

@@ -15,6 +15,7 @@
  */
 import Foundation
 import NIO
+import NIOTransportServices
 import NIOHTTP2
 import NIOSSL
 import NIOTLS
@@ -633,13 +634,14 @@ extension Channel {
     connectionKeepalive: ClientConnectionKeepalive,
     connectionIdleTimeout: TimeAmount,
     errorDelegate: ClientErrorDelegate?,
+    requiresZeroLengthWriteWorkaround: Bool,
     logger: Logger
   ) -> EventLoopFuture<Void> {
     let tlsConfigured = tlsConfiguration.map {
       self.configureTLS($0, serverHostname: tlsServerHostname, errorDelegate: errorDelegate, logger: logger)
     }
 
-    return (tlsConfigured ?? self.eventLoop.makeSucceededFuture(())).flatMap {
+    let configuration: EventLoopFuture<Void> = (tlsConfigured ?? self.eventLoop.makeSucceededFuture(())).flatMap {
       self.configureHTTP2Pipeline(mode: .client, targetWindowSize: httpTargetWindowSize)
     }.flatMap { _ in
       return self.pipeline.handler(type: NIOHTTP2Handler.self).flatMap { http2Handler in
@@ -656,6 +658,19 @@ extension Channel {
         return self.pipeline.addHandler(errorHandler)
       }
     }
+
+    #if canImport(Network)
+    // This availability guard is arguably unnecessary, but we add it anyway.
+    if requiresZeroLengthWriteWorkaround, #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
+      return configuration.flatMap {
+        self.pipeline.addHandler(NIOFilterEmptyWritesHandler(), position: .first)
+      }
+    } else {
+      return configuration
+    }
+    #else
+    return configuration
+    #endif
   }
 
   func configureGRPCClient(

Sources/GRPC/ConnectionManager.swift

@@ -611,6 +611,7 @@ extension ConnectionManager {
           connectionKeepalive: configuration.connectionKeepalive,
           connectionIdleTimeout: configuration.connectionIdleTimeout,
           errorDelegate: configuration.errorDelegate,
+          requiresZeroLengthWriteWorkaround: PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.eventLoop, hasTLS: configuration.tls != nil),
           logger: self.logger
         )
 

Sources/GRPC/PlatformSupport.swift

@@ -227,4 +227,24 @@ public enum PlatformSupport {
     logger.debug("creating a ServerBootstrap")
     return ServerBootstrap(group: group)
   }
+
+  /// Determines whether we may need to work around an issue in Network.framework with zero-length writes.
+  ///
+  /// See https://github.com/apple/swift-nio-transport-services/pull/72 for more.
+  static func requiresZeroLengthWriteWorkaround(group: EventLoopGroup, hasTLS: Bool) -> Bool {
+    #if canImport(Network)
+    if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
+      if group is NIOTSEventLoopGroup || group is QoSEventLoop {
+        // We need the zero-length write workaround on NIOTS when not using TLS.
+        return !hasTLS
+      } else {
+        return false
+      }
+    } else {
+      return false
+    }
+    #else
+    return false
+    #endif
+  }
 }

Sources/GRPC/Server.swift

@@ -15,6 +15,7 @@
  */
 import Foundation
 import NIO
+import NIOTransportServices
 import NIOHTTP1
 import NIOHTTP2
 import NIOSSL
@@ -115,7 +116,7 @@ public final class Server {
           return channel.pipeline.addHandler(handler)
         }
 
-        let configured: EventLoopFuture<Void>
+        var configured: EventLoopFuture<Void>
 
         if let tls = configuration.tls {
           configured = channel.configureTLS(configuration: tls).flatMap {
@@ -125,6 +126,14 @@ public final class Server {
           configured = channel.pipeline.addHandler(protocolSwitcher)
         }
 
+        // Work around the zero length write issue, if needed.
+        let requiresZeroLengthWorkaround = PlatformSupport.requiresZeroLengthWriteWorkaround(group: configuration.eventLoopGroup, hasTLS: configuration.tls != nil)
+        if requiresZeroLengthWorkaround, #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
+          configured = configured.flatMap {
+            channel.pipeline.addHandler(NIOFilterEmptyWritesHandler())
+          }
+        }
+
         // Add the debug initializer, if there is one.
         if let debugAcceptedChannelInitializer = configuration.debugChannelInitializer {
           return configured.flatMap {

Tests/GRPCTests/PlatformSupportTests.swift

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 import Foundation
-import GRPC
+@testable import GRPC
 import NIO
 import NIOTransportServices
 import XCTest
@@ -128,4 +128,28 @@ class PlatformSupportTests: GRPCTestCase {
     XCTAssertTrue(bootstrap is NIOTSListenerBootstrap)
     #endif
   }
+
+  func testRequiresZeroLengthWorkaroundWithMTELG() {
+    self.group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
+
+    // No MTELG or individual loop requires the workaround.
+    XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group, hasTLS: true))
+    XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group, hasTLS: false))
+    XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group.next(), hasTLS: true))
+    XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group.next(), hasTLS: false))
+  }
+
+  func testRequiresZeroLengthWorkaroundWithNetworkFramework() {
+    // If we don't have Network.framework we can't test this.
+    #if canImport(Network)
+    guard #available(macOS 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
+    self.group = NIOTSEventLoopGroup(loopCount: 1)
+
+    // We require the workaround for any of these loops when TLS is not enabled.
+    XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group, hasTLS: true))
+    XCTAssertTrue(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group, hasTLS: false))
+    XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group.next(), hasTLS: true))
+    XCTAssertTrue(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group.next(), hasTLS: false))
+    #endif
+  }
 }

Tests/GRPCTests/XCTestManifests.swift

@@ -765,6 +765,8 @@ extension PlatformSupportTests {
         ("testMakeServerBootstrapReturnsNIOTSListenerBootstrapForQoSEventLoop", testMakeServerBootstrapReturnsNIOTSListenerBootstrapForQoSEventLoop),
         ("testMakeServerBootstrapReturnsServerBootstrapForEventLoop", testMakeServerBootstrapReturnsServerBootstrapForEventLoop),
         ("testMakeServerBootstrapReturnsServerBootstrapForMultiThreadedGroup", testMakeServerBootstrapReturnsServerBootstrapForMultiThreadedGroup),
+        ("testRequiresZeroLengthWorkaroundWithMTELG", testRequiresZeroLengthWorkaroundWithMTELG),
+        ("testRequiresZeroLengthWorkaroundWithNetworkFramework", testRequiresZeroLengthWorkaroundWithNetworkFramework),
     ]
 }
 
@@ -895,6 +897,18 @@ extension TimeLimitTests {
     ]
 }
 
+extension ZeroLengthWriteTests {
+    // DO NOT MODIFY: This is autogenerated, use:
+    //   `swift test --generate-linuxmain`
+    // to regenerate.
+    static let __allTests__ZeroLengthWriteTests = [
+        ("testZeroLengthWriteTestNetworkFrameworkInsecure", testZeroLengthWriteTestNetworkFrameworkInsecure),
+        ("testZeroLengthWriteTestNetworkFrameworkSecure", testZeroLengthWriteTestNetworkFrameworkSecure),
+        ("testZeroLengthWriteTestPosixInsecure", testZeroLengthWriteTestPosixInsecure),
+        ("testZeroLengthWriteTestPosixSecure", testZeroLengthWriteTestPosixSecure),
+    ]
+}
+
 extension ZlibTests {
     // DO NOT MODIFY: This is autogenerated, use:
     //   `swift test --generate-linuxmain`
@@ -973,6 +987,7 @@ public func __allTests() -> [XCTestCaseEntry] {
         testCase(StopwatchTests.__allTests__StopwatchTests),
         testCase(StreamingRequestClientCallTests.__allTests__StreamingRequestClientCallTests),
         testCase(TimeLimitTests.__allTests__TimeLimitTests),
+        testCase(ZeroLengthWriteTests.__allTests__ZeroLengthWriteTests),
         testCase(ZlibTests.__allTests__ZlibTests),
     ]
 }