Skip to content

Commit 4fe97e8

Browse files
committed
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.
1 parent fa00728 commit 4fe97e8

File tree

7 files changed

+251
-3
lines changed

7 files changed

+251
-3
lines changed

Sources/GRPC/ClientConnection.swift

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
import Foundation
1717
import NIO
18+
import NIOTransportServices
1819
import NIOHTTP2
1920
import NIOSSL
2021
import NIOTLS
@@ -633,13 +634,14 @@ extension Channel {
633634
connectionKeepalive: ClientConnectionKeepalive,
634635
connectionIdleTimeout: TimeAmount,
635636
errorDelegate: ClientErrorDelegate?,
637+
requiresZeroLengthWriteWorkaround: Bool,
636638
logger: Logger
637639
) -> EventLoopFuture<Void> {
638640
let tlsConfigured = tlsConfiguration.map {
639641
self.configureTLS($0, serverHostname: tlsServerHostname, errorDelegate: errorDelegate, logger: logger)
640642
}
641643

642-
return (tlsConfigured ?? self.eventLoop.makeSucceededFuture(())).flatMap {
644+
let configuration: EventLoopFuture<Void> = (tlsConfigured ?? self.eventLoop.makeSucceededFuture(())).flatMap {
643645
self.configureHTTP2Pipeline(mode: .client, targetWindowSize: httpTargetWindowSize)
644646
}.flatMap { _ in
645647
return self.pipeline.handler(type: NIOHTTP2Handler.self).flatMap { http2Handler in
@@ -656,6 +658,19 @@ extension Channel {
656658
return self.pipeline.addHandler(errorHandler)
657659
}
658660
}
661+
662+
#if canImport(Network)
663+
// This availability guard is arguably unnecessary, but we add it anyway.
664+
if requiresZeroLengthWriteWorkaround, #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
665+
return configuration.flatMap {
666+
self.pipeline.addHandler(NIOFilterEmptyWritesHandler(), position: .first)
667+
}
668+
} else {
669+
return configuration
670+
}
671+
#else
672+
return configuration
673+
#endif
659674
}
660675

661676
func configureGRPCClient(

Sources/GRPC/ConnectionManager.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,6 +611,7 @@ extension ConnectionManager {
611611
connectionKeepalive: configuration.connectionKeepalive,
612612
connectionIdleTimeout: configuration.connectionIdleTimeout,
613613
errorDelegate: configuration.errorDelegate,
614+
requiresZeroLengthWriteWorkaround: PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.eventLoop, hasTLS: configuration.tls != nil),
614615
logger: self.logger
615616
)
616617

Sources/GRPC/PlatformSupport.swift

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,4 +227,21 @@ public enum PlatformSupport {
227227
logger.debug("creating a ServerBootstrap")
228228
return ServerBootstrap(group: group)
229229
}
230+
231+
static func requiresZeroLengthWriteWorkaround(group: EventLoopGroup, hasTLS: Bool) -> Bool {
232+
#if canImport(Network)
233+
if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
234+
if group is NIOTSEventLoopGroup || group is QoSEventLoop {
235+
// We need the zero-length write workaround on NIOTS when not using TLS.
236+
return !hasTLS
237+
} else {
238+
return false
239+
}
240+
} else {
241+
return false
242+
}
243+
#else
244+
return false
245+
#endif
246+
}
230247
}

Sources/GRPC/Server.swift

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
import Foundation
1717
import NIO
18+
import NIOTransportServices
1819
import NIOHTTP1
1920
import NIOHTTP2
2021
import NIOSSL
@@ -115,7 +116,7 @@ public final class Server {
115116
return channel.pipeline.addHandler(handler)
116117
}
117118

118-
let configured: EventLoopFuture<Void>
119+
var configured: EventLoopFuture<Void>
119120

120121
if let tls = configuration.tls {
121122
configured = channel.configureTLS(configuration: tls).flatMap {
@@ -125,6 +126,14 @@ public final class Server {
125126
configured = channel.pipeline.addHandler(protocolSwitcher)
126127
}
127128

129+
// Work around the zero length write issue, if needed.
130+
let requiresZeroLengthWorkaround = PlatformSupport.requiresZeroLengthWriteWorkaround(group: configuration.eventLoopGroup, hasTLS: configuration.tls != nil)
131+
if requiresZeroLengthWorkaround, #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
132+
configured = configured.flatMap {
133+
channel.pipeline.addHandler(NIOFilterEmptyWritesHandler())
134+
}
135+
}
136+
128137
// Add the debug initializer, if there is one.
129138
if let debugAcceptedChannelInitializer = configuration.debugChannelInitializer {
130139
return configured.flatMap {

Tests/GRPCTests/PlatformSupportTests.swift

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
* limitations under the License.
1515
*/
1616
import Foundation
17-
import GRPC
17+
@testable import GRPC
1818
import NIO
1919
import NIOTransportServices
2020
import XCTest
@@ -128,4 +128,28 @@ class PlatformSupportTests: GRPCTestCase {
128128
XCTAssertTrue(bootstrap is NIOTSListenerBootstrap)
129129
#endif
130130
}
131+
132+
func testRequiresZeroLengthWorkaroundWithMTELG() {
133+
self.group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
134+
135+
// No MTELG or individual loop requires the workaround.
136+
XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group, hasTLS: true))
137+
XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group, hasTLS: false))
138+
XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group.next(), hasTLS: true))
139+
XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group.next(), hasTLS: false))
140+
}
141+
142+
func testRequiresZeroLengthWorkaroundWithNetworkFramework() {
143+
// If we don't have Network.framework we can't test this.
144+
#if canImport(Network)
145+
guard #available(macOS 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
146+
self.group = NIOTSEventLoopGroup(loopCount: 1)
147+
148+
// We require the workaround for any of these loops when TLS is not enabled.
149+
XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group, hasTLS: true))
150+
XCTAssertTrue(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group, hasTLS: false))
151+
XCTAssertFalse(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group.next(), hasTLS: true))
152+
XCTAssertTrue(PlatformSupport.requiresZeroLengthWriteWorkaround(group: self.group.next(), hasTLS: false))
153+
#endif
154+
}
131155
}

Tests/GRPCTests/XCTestManifests.swift

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,8 @@ extension PlatformSupportTests {
765765
("testMakeServerBootstrapReturnsNIOTSListenerBootstrapForQoSEventLoop", testMakeServerBootstrapReturnsNIOTSListenerBootstrapForQoSEventLoop),
766766
("testMakeServerBootstrapReturnsServerBootstrapForEventLoop", testMakeServerBootstrapReturnsServerBootstrapForEventLoop),
767767
("testMakeServerBootstrapReturnsServerBootstrapForMultiThreadedGroup", testMakeServerBootstrapReturnsServerBootstrapForMultiThreadedGroup),
768+
("testRequiresZeroLengthWorkaroundWithMTELG", testRequiresZeroLengthWorkaroundWithMTELG),
769+
("testRequiresZeroLengthWorkaroundWithNetworkFramework", testRequiresZeroLengthWorkaroundWithNetworkFramework),
768770
]
769771
}
770772

@@ -895,6 +897,18 @@ extension TimeLimitTests {
895897
]
896898
}
897899

900+
extension ZeroLengthWriteTests {
901+
// DO NOT MODIFY: This is autogenerated, use:
902+
// `swift test --generate-linuxmain`
903+
// to regenerate.
904+
static let __allTests__ZeroLengthWriteTests = [
905+
("testZeroLengthWriteTestNetworkFrameworkInsecure", testZeroLengthWriteTestNetworkFrameworkInsecure),
906+
("testZeroLengthWriteTestNetworkFrameworkSecure", testZeroLengthWriteTestNetworkFrameworkSecure),
907+
("testZeroLengthWriteTestPosixInsecure", testZeroLengthWriteTestPosixInsecure),
908+
("testZeroLengthWriteTestPosixSecure", testZeroLengthWriteTestPosixSecure),
909+
]
910+
}
911+
898912
extension ZlibTests {
899913
// DO NOT MODIFY: This is autogenerated, use:
900914
// `swift test --generate-linuxmain`
@@ -973,6 +987,7 @@ public func __allTests() -> [XCTestCaseEntry] {
973987
testCase(StopwatchTests.__allTests__StopwatchTests),
974988
testCase(StreamingRequestClientCallTests.__allTests__StreamingRequestClientCallTests),
975989
testCase(TimeLimitTests.__allTests__TimeLimitTests),
990+
testCase(ZeroLengthWriteTests.__allTests__ZeroLengthWriteTests),
976991
testCase(ZlibTests.__allTests__ZlibTests),
977992
]
978993
}
Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
/*
2+
* Copyright 2020, gRPC Authors All rights reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
import Dispatch
17+
import Foundation
18+
import NIO
19+
import NIOSSL
20+
import NIOTransportServices
21+
import GRPC
22+
import GRPCSampleData
23+
import EchoModel
24+
import EchoImplementation
25+
import XCTest
26+
27+
final class ZeroLengthWriteTests: GRPCTestCase {
28+
func clientBuilder(group: EventLoopGroup, secure: Bool, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) -> ClientConnection.Builder {
29+
if secure {
30+
return ClientConnection.secure(group: group)
31+
.withTLS(trustRoots: .certificates([SampleCertificate.ca.certificate]))
32+
.withDebugChannelInitializer(debugInitializer)
33+
} else {
34+
return ClientConnection.insecure(group: group)
35+
.withDebugChannelInitializer(debugInitializer)
36+
}
37+
}
38+
39+
func serverBuilder(group: EventLoopGroup, secure: Bool, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) -> Server.Builder {
40+
if secure {
41+
return Server.secure(
42+
group: group,
43+
certificateChain: [SampleCertificate.server.certificate],
44+
privateKey: SamplePrivateKey.server
45+
).withTLS(trustRoots: .certificates([SampleCertificate.ca.certificate]))
46+
.withDebugChannelInitializer(debugInitializer)
47+
} else {
48+
return Server.insecure(group: group)
49+
.withDebugChannelInitializer(debugInitializer)
50+
}
51+
}
52+
53+
func makeServer(group: EventLoopGroup, secure: Bool, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) throws -> Server {
54+
return try self.serverBuilder(group: group, secure: secure, debugInitializer: debugInitializer)
55+
.withServiceProviders([self.makeEchoProvider()])
56+
.withLogger(self.serverLogger)
57+
.bind(host: "localhost", port: 0)
58+
.wait()
59+
}
60+
61+
func makeClientConnection(group: EventLoopGroup, secure: Bool, port: Int, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) throws -> ClientConnection {
62+
return self.clientBuilder(group: group, secure: secure, debugInitializer: debugInitializer)
63+
.withBackgroundActivityLogger(self.clientLogger)
64+
.connect(host: "localhost", port: port)
65+
}
66+
67+
func makeEchoProvider() -> Echo_EchoProvider { return EchoProvider() }
68+
69+
func makeEchoClient(group: EventLoopGroup, secure: Bool, port: Int, debugInitializer: @escaping (Channel) -> EventLoopFuture<Void>) throws -> Echo_EchoClient {
70+
return Echo_EchoClient(
71+
channel: try self.makeClientConnection(group: group, secure: secure, port: port, debugInitializer: debugInitializer),
72+
defaultCallOptions: self.callOptionsWithLogger
73+
)
74+
}
75+
76+
func zeroLengthWriteExpectation() -> XCTestExpectation {
77+
let expectation = self.expectation(description: "Expecting zero length write workaround")
78+
expectation.expectedFulfillmentCount = 1
79+
expectation.assertForOverFulfill = true
80+
return expectation
81+
}
82+
83+
func noZeroLengthWriteExpectation() -> XCTestExpectation {
84+
let expectation = self.expectation(description: "Not expecting zero length write workaround")
85+
expectation.isInverted = true
86+
return expectation
87+
}
88+
89+
func debugPipelineExpectation(_ expectation: XCTestExpectation) -> (Channel) -> EventLoopFuture<Void> {
90+
return { channel in
91+
channel.pipeline.handler(type: NIOFilterEmptyWritesHandler.self).map { _ in
92+
expectation.fulfill()
93+
}.recover { _ in () }
94+
}
95+
}
96+
97+
private func _runTest(networkPreference: NetworkPreference, secure: Bool, clientExpectation: XCTestExpectation, serverExpectation: XCTestExpectation) {
98+
// We can only run this test on platforms where the zero-length write workaround _could_ be added.
99+
#if canImport(Network)
100+
guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
101+
let group = PlatformSupport.makeEventLoopGroup(
102+
loopCount: 1,
103+
networkPreference: networkPreference)
104+
let server = try! self.makeServer(group: group, secure: secure, debugInitializer: self.debugPipelineExpectation(serverExpectation))
105+
106+
defer {
107+
XCTAssertNoThrow(try server.close().wait())
108+
XCTAssertNoThrow(try group.syncShutdownGracefully())
109+
}
110+
111+
let port = server.channel.localAddress!.port!
112+
let client = try! self.makeEchoClient(group: group, secure: secure, port: port, debugInitializer: self.debugPipelineExpectation(clientExpectation))
113+
defer {
114+
XCTAssertNoThrow(try client.channel.close().wait())
115+
}
116+
117+
// We need to wait here to confirm that the RPC completes. All expectations should have completed by then.
118+
let call = client.get(Echo_EchoRequest(text: "foo bar baz"))
119+
XCTAssertNoThrow(try call.status.wait())
120+
self.waitForExpectations(timeout: 1.0)
121+
#endif
122+
}
123+
124+
func testZeroLengthWriteTestPosixSecure() throws {
125+
// We can only run this test on platforms where the zero-length write workaround _could_ be added.
126+
#if canImport(Network)
127+
guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
128+
129+
let serverExpectation = self.noZeroLengthWriteExpectation()
130+
let clientExpectation = self.noZeroLengthWriteExpectation()
131+
self._runTest(networkPreference: .userDefined(.posix), secure: true, clientExpectation: clientExpectation, serverExpectation: serverExpectation)
132+
#endif
133+
}
134+
135+
func testZeroLengthWriteTestPosixInsecure() throws {
136+
// We can only run this test on platforms where the zero-length write workaround _could_ be added.
137+
#if canImport(Network)
138+
guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
139+
140+
let serverExpectation = self.noZeroLengthWriteExpectation()
141+
let clientExpectation = self.noZeroLengthWriteExpectation()
142+
self._runTest(networkPreference: .userDefined(.posix), secure: false, clientExpectation: clientExpectation, serverExpectation: serverExpectation)
143+
#endif
144+
}
145+
146+
func testZeroLengthWriteTestNetworkFrameworkSecure() throws {
147+
// We can only run this test on platforms where the zero-length write workaround _could_ be added.
148+
#if canImport(Network)
149+
guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
150+
151+
let serverExpectation = self.noZeroLengthWriteExpectation()
152+
let clientExpectation = self.noZeroLengthWriteExpectation()
153+
self._runTest(networkPreference: .userDefined(.networkFramework), secure: true, clientExpectation: clientExpectation, serverExpectation: serverExpectation)
154+
#endif
155+
}
156+
157+
func testZeroLengthWriteTestNetworkFrameworkInsecure() throws {
158+
// We can only run this test on platforms where the zero-length write workaround _could_ be added.
159+
#if canImport(Network)
160+
guard #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) else { return }
161+
162+
let serverExpectation = self.zeroLengthWriteExpectation()
163+
let clientExpectation = self.zeroLengthWriteExpectation()
164+
self._runTest(networkPreference: .userDefined(.networkFramework), secure: false, clientExpectation: clientExpectation, serverExpectation: serverExpectation)
165+
#endif
166+
}
167+
}

0 commit comments

Comments
 (0)