Mitigate zero-length writes issue. (#917)

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.

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),
     ]
 }

Tests/GRPCTests/ZeroLengthWriteTests.swift

@@ -0,0 +1,225 @@
+/*
+ * Copyright 2020, gRPC Authors All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import Dispatch
+import Foundation
+import NIO
+import NIOSSL
+import NIOTransportServices
+import GRPC
+import GRPCSampleData
+import EchoModel
+import EchoImplementation
+import XCTest
+
+final class ZeroLengthWriteTests: GRPCTestCase {
+  func clientBuilder(group: EventLoopGroup, secure: Bool, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) -> ClientConnection.Builder {
+    if secure {
+      return ClientConnection.secure(group: group)
+        .withTLS(trustRoots: .certificates([SampleCertificate.ca.certificate]))
+        .withDebugChannelInitializer(debugInitializer)
+    } else {
+      return ClientConnection.insecure(group: group)
+        .withDebugChannelInitializer(debugInitializer)
+    }
+  }
+
+  func serverBuilder(group: EventLoopGroup, secure: Bool, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) -> Server.Builder {
+    if secure {
+      return Server.secure(
+        group: group,
+        certificateChain: [SampleCertificate.server.certificate],
+        privateKey: SamplePrivateKey.server
+      ).withTLS(trustRoots: .certificates([SampleCertificate.ca.certificate]))
+      .withDebugChannelInitializer(debugInitializer)
+    } else {
+      return Server.insecure(group: group)
+        .withDebugChannelInitializer(debugInitializer)
+    }
+  }
+
+  func makeServer(group: EventLoopGroup, secure: Bool, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) throws -> Server {
+    return try self.serverBuilder(group: group, secure: secure, debugInitializer: debugInitializer)
+      .withServiceProviders([self.makeEchoProvider()])
+      .withLogger(self.serverLogger)
+      .bind(host: "localhost", port: 0)
+      .wait()
+  }
+
+  func makeClientConnection(group: EventLoopGroup, secure: Bool, port: Int, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) throws -> ClientConnection {
+    return self.clientBuilder(group: group, secure: secure, debugInitializer: debugInitializer)
+      .withBackgroundActivityLogger(self.clientLogger)
+      .connect(host: "localhost", port: port)
+  }
+
+  func makeEchoProvider() -> Echo_EchoProvider { return EchoProvider() }
+
+  func makeEchoClient(group: EventLoopGroup, secure: Bool, port: Int, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) throws -> Echo_EchoClient {
+    return Echo_EchoClient(
+      channel: try self.makeClientConnection(group: group, secure: secure, port: port, debugInitializer: debugInitializer),
+      defaultCallOptions: self.callOptionsWithLogger
+    )
+  }
+
+  func zeroLengthWriteExpectation() -> XCTestExpectation {
+    let expectation = self.expectation(description: "Expecting zero length write workaround")
+    expectation.expectedFulfillmentCount = 1
+    expectation.assertForOverFulfill = true
+    return expectation
+  }
+
+  func noZeroLengthWriteExpectation() -> XCTestExpectation {
+    let expectation = self.expectation(description: "Not expecting zero length write workaround")
+    expectation.expectedFulfillmentCount = 1
+    expectation.assertForOverFulfill = true
+    return expectation
+  }
+
+  func debugPipelineExpectation(_ callback: @escaping (Result<NIOFilterEmptyWritesHandler, Error>) -> Void) -> (Channel) -> EventLoopFuture<Void> {
+    return { channel in
+      channel.pipeline.handler(type: NIOFilterEmptyWritesHandler.self).always { result in
+        callback(result)
+      }.map { _ in () }.recover { _ in () }
+    }
+  }
+
+  private func _runTest(
+    networkPreference: NetworkPreference,
+    secure: Bool,
+    clientHandlerCallback: @escaping (Result<NIOFilterEmptyWritesHandler, Error>) -> Void,
+    serverHandlerCallback: @escaping (Result<NIOFilterEmptyWritesHandler, Error>) -> Void
+  ) {
+    // We can only run this test on platforms where the zero-length write workaround _could_ be added.
+    #if canImport(Network)
+    guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
+    let group = PlatformSupport.makeEventLoopGroup(
+      loopCount: 1,
+      networkPreference: networkPreference)
+    let server = try! self.makeServer(group: group, secure: secure, debugInitializer: self.debugPipelineExpectation(serverHandlerCallback))
+
+    defer {
+      XCTAssertNoThrow(try server.close().wait())
+      XCTAssertNoThrow(try group.syncShutdownGracefully())
+    }
+
+    let port = server.channel.localAddress!.port!
+    let client = try! self.makeEchoClient(group: group, secure: secure, port: port, debugInitializer: self.debugPipelineExpectation(clientHandlerCallback))
+    defer {
+      XCTAssertNoThrow(try client.channel.close().wait())
+    }
+
+    // We need to wait here to confirm that the RPC completes. All expectations should have completed by then.
+    let call = client.get(Echo_EchoRequest(text: "foo bar baz"))
+    XCTAssertNoThrow(try call.status.wait())
+    self.waitForExpectations(timeout: 1.0)
+    #endif
+  }
+
+  func testZeroLengthWriteTestPosixSecure() throws {
+    // We can only run this test on platforms where the zero-length write workaround _could_ be added.
+    #if canImport(Network)
+    guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
+
+    let serverExpectation = self.noZeroLengthWriteExpectation()
+    let clientExpectation = self.noZeroLengthWriteExpectation()
+    self._runTest(
+      networkPreference: .userDefined(.posix),
+      secure: true,
+      clientHandlerCallback: { result in
+        if case .failure = result {
+          clientExpectation.fulfill()
+        }
+      },
+      serverHandlerCallback: { result in
+        if case .failure = result {
+          serverExpectation.fulfill()
+        }
+      }
+    )
+    #endif
+  }
+
+  func testZeroLengthWriteTestPosixInsecure() throws {
+    // We can only run this test on platforms where the zero-length write workaround _could_ be added.
+    #if canImport(Network)
+    guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
+
+    let serverExpectation = self.noZeroLengthWriteExpectation()
+    let clientExpectation = self.noZeroLengthWriteExpectation()
+    self._runTest(
+      networkPreference: .userDefined(.posix),
+      secure: false,
+      clientHandlerCallback: { result in
+        if case .failure = result {
+          clientExpectation.fulfill()
+        }
+      },
+      serverHandlerCallback: { result in
+        if case .failure = result {
+          serverExpectation.fulfill()
+        }
+      }
+    )
+    #endif
+  }
+
+  func testZeroLengthWriteTestNetworkFrameworkSecure() throws {
+    // We can only run this test on platforms where the zero-length write workaround _could_ be added.
+    #if canImport(Network)
+    guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
+
+    let serverExpectation = self.noZeroLengthWriteExpectation()
+    let clientExpectation = self.noZeroLengthWriteExpectation()
+    self._runTest(
+      networkPreference: .userDefined(.networkFramework),
+      secure: true,
+      clientHandlerCallback: { result in
+        if case .failure = result {
+          clientExpectation.fulfill()
+        }
+      },
+      serverHandlerCallback: { result in
+        if case .failure = result {
+          serverExpectation.fulfill()
+        }
+      }
+    )
+    #endif
+  }
+
+  func testZeroLengthWriteTestNetworkFrameworkInsecure() throws {
+    // We can only run this test on platforms where the zero-length write workaround _could_ be added.
+    #if canImport(Network)
+    guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
+
+    let serverExpectation = self.zeroLengthWriteExpectation()
+    let clientExpectation = self.zeroLengthWriteExpectation()
+    self._runTest(
+      networkPreference: .userDefined(.networkFramework),
+      secure: false,
+      clientHandlerCallback: { result in
+        if case .success = result {
+          clientExpectation.fulfill()
+        }
+      },
+      serverHandlerCallback: { result in
+        if case .success = result {
+          serverExpectation.fulfill()
+        }
+      }
+    )
+    #endif
+  }
+}