Skip to content

Commit 8c84285

Browse files
Avoid double continuation resume in URLSession delegate in corner case (#28)
### Motivation If the request completes with error after the response has already been received we can potentially resume a continuation twice. This was shown by running some of the tests in long loop. ### Modifications - Add a shorter test that more reliably reproduced the issue. - Add the fix: to set the continuation to `nil` after resuming it, while still holding the lock. ### Result - Removed a source of a potential runtime crash. ### Test Plan - The newly added test, which reliably failed, now reliably passes.
1 parent aee2c63 commit 8c84285

File tree

2 files changed

+18
-2
lines changed

2 files changed

+18
-2
lines changed

Sources/OpenAPIURLSession/URLSessionBidirectionalStreaming/BidirectionalStreamingURLSessionDelegate.swift

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,8 @@ final class BidirectionalStreamingURLSessionDelegate: NSObject, URLSessionTaskDe
164164
{
165165
callbackLock.withLock {
166166
debug("Task delegate: didReceive response")
167-
self.responseContinuation?.resume(returning: response)
167+
responseContinuation?.resume(returning: response)
168+
responseContinuation = nil
168169
return .allow
169170
}
170171
}
@@ -173,7 +174,10 @@ final class BidirectionalStreamingURLSessionDelegate: NSObject, URLSessionTaskDe
173174
callbackLock.withLock {
174175
debug("Task delegate: didCompleteWithError (error: \(String(describing: error)))")
175176
responseBodyStreamSource.finish(throwing: error)
176-
if let error { responseContinuation?.resume(throwing: error) }
177+
if let error {
178+
responseContinuation?.resume(throwing: error)
179+
responseContinuation = nil
180+
}
177181
}
178182
}
179183
}

Tests/OpenAPIURLSessionTests/URLSessionBidirectionalStreamingTests/URLSessionBidirectionalStreamingTests.swift

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,15 @@ class URLSessionBidirectionalStreamingTests: XCTestCase {
238238
}
239239
}
240240

241+
func testStreamingDownload_1kChunk_100Chunks_100BDownloadWatermark() async throws {
242+
try await testStreamingDownload(
243+
responseChunk: (1...1024).map { _ in .random(in: (.min..<(.max))) }[...],
244+
numResponseChunks: 100,
245+
responseStreamWatermarks: (low: 100, high: 100),
246+
verification: .none
247+
)
248+
}
249+
241250
func testStreamingDownload_1kChunk_10kChunks_100BDownloadWatermark() async throws {
242251
try await testStreamingDownload(
243252
responseChunk: (1...1024).map { _ in .random(in: (.min..<(.max))) }[...],
@@ -306,6 +315,8 @@ class URLSessionBidirectionalStreamingTests: XCTestCase {
306315
case count
307316
// Add some artificial delay to simulate business logic to show how the backpressure mechanism works (or not).
308317
case delay(TimeAmount)
318+
// Do no verification: useful for just pulling as fast as possible for testing for races.
319+
case none
309320
}
310321

311322
func testStreamingDownload(
@@ -389,6 +400,7 @@ class URLSessionBidirectionalStreamingTests: XCTestCase {
389400
debug("Client doing fake work for \(delay)s")
390401
try await Task.sleep(nanoseconds: UInt64(delay.nanoseconds))
391402
}
403+
case .none: break
392404
}
393405

394406
group.cancelAll()

0 commit comments

Comments
 (0)