mirror of
https://github.com/swift-server/swift-aws-lambda-runtime.git
synced 2026-06-02 07:27:33 +00:00
# Fix test hangs caused by Pool cancellation race conditions ## Summary This PR fixes two related race conditions in `Lambda+LocalServer+Pool.swift` that were causing the test suite to hang approximately 10% of the time. ## Problem The test suite exhibited intermittent hangs (~10% frequency) due to two bugs in the Pool implementation: 1. **Individual task cancellation bug**: When one task waiting for a specific `requestId` was cancelled, the cancellation handler would incorrectly cancel ALL waiting tasks instead of just the cancelled one. 2. **Server shutdown hang**: When the server shut down, waiting continuations in the pools were never cancelled, causing handlers to wait indefinitely for responses that would never arrive. ## Root Causes ### Root Cause #1: Cancellation Handler Removes ALL Continuations The `onCancel` handler in `Pool._next()` was removing all continuations from the `waitingForSpecific` dictionary when any single task was cancelled: ```swift onCancel: { // BUG: Removes ALL continuations, not just the cancelled task's for continuation in state.waitingForSpecific.values { toCancel.append(continuation) } state.waitingForSpecific.removeAll() } ``` This caused unrelated concurrent invocations to fail with `CancellationError` when one client cancelled their request. ### Root Cause #2: No Pool Cleanup During Server Shutdown When the server shut down (e.g., test completes), the task group was cancelled but the pools' waiting continuations were never notified. The `/invoke` endpoint handlers would continue waiting for responses that would never arrive because the Lambda function had stopped. ## Solution ### Fix #1: Only Remove Specific Continuation on Cancellation Modified the cancellation handler to only remove the continuation for the specific cancelled task: ```swift onCancel: { // Only remove THIS task's continuation let continuationToCancel = self.lock.withLock { state -> CheckedContinuation<T, any Error>? in if let requestId = requestId { return state.waitingForSpecific.removeValue(forKey: requestId) } else { let cont = state.waitingForAny state.waitingForAny = nil return cont } } continuationToCancel?.resume(throwing: CancellationError()) } ``` ### Fix #2: Add Pool Cleanup During Server Shutdown Added `cancelAll()` method to the Pool class and call it during server shutdown: ```swift func cancelAll() { let continuationsToCancel = self.lock.withLock { state -> [CheckedContinuation<T, any Error>] in var toCancel: [CheckedContinuation<T, any Error>] = [] if let continuation = state.waitingForAny { toCancel.append(continuation) state.waitingForAny = nil } for continuation in state.waitingForSpecific.values { toCancel.append(continuation) } state.waitingForSpecific.removeAll() return toCancel } for continuation in continuationsToCancel { continuation.resume(throwing: CancellationError()) } } ``` Called during server shutdown: ```swift let serverOrHandlerResult1 = await group.next()! group.cancelAll() // Cancel all waiting continuations in the pools to prevent hangs server.invocationPool.cancelAll() server.responsePool.cancelAll() ``` ## Changes ### Modified Files - **Sources/AWSLambdaRuntime/HTTPServer/Lambda+LocalServer+Pool.swift** - Fixed cancellation handler in `_next()` to only remove specific continuation - Added `cancelAll()` method for server shutdown cleanup - **Sources/AWSLambdaRuntime/HTTPServer/Lambda+LocalServer.swift** - Call `cancelAll()` on both pools during server shutdown ### New Files - **Tests/AWSLambdaRuntimeTests/LocalServerPoolCancellationTests.swift** - Added comprehensive test suite with 3 tests - `testCancellationOnlyAffectsOwnTask`: Verifies only the cancelled task receives CancellationError - `testConcurrentInvocationsWithCancellation`: Tests real-world scenario with 5 concurrent invocations - `testFIFOModeCancellation`: Ensures FIFO mode cancellation works correctly ## Testing ### Before Fix - Test suite hung ~10% of the time - When 1 task was cancelled, all 5 concurrent tasks received `CancellationError` - Streaming tests would occasionally hang during shutdown ### After Fix - All 91 tests pass consistently without hangs - When 1 task is cancelled, only that specific task receives `CancellationError` - Other tasks continue waiting normally - Server shutdown properly cleans up all waiting continuations - Multiple consecutive test runs confirm stability ### Test Coverage The new test suite reproduces both bugs and verifies the fixes: 1. **testCancellationOnlyAffectsOwnTask**: Creates 3 tasks waiting for different requestIds, cancels only one, and verifies the others are not affected 2. **testConcurrentInvocationsWithCancellation**: Simulates 5 concurrent invocations with one cancellation 3. **testFIFOModeCancellation**: Tests FIFO mode to ensure it still works correctly --------- Co-authored-by: Sebastien Stormacq <stormacq@amazon.lu>
211 lines
8.9 KiB
Swift
211 lines
8.9 KiB
Swift
//===----------------------------------------------------------------------===//
|
|
//
|
|
// This source file is part of the SwiftAWSLambdaRuntime open source project
|
|
//
|
|
// Copyright SwiftAWSLambdaRuntime project authors
|
|
// Copyright (c) Amazon.com, Inc. or its affiliates.
|
|
// Licensed under Apache License v2.0
|
|
//
|
|
// See LICENSE.txt for license information
|
|
// See CONTRIBUTORS.txt for the list of SwiftAWSLambdaRuntime project authors
|
|
//
|
|
// SPDX-License-Identifier: Apache-2.0
|
|
//
|
|
//===----------------------------------------------------------------------===//
|
|
|
|
#if LocalServerSupport
|
|
import DequeModule
|
|
import Synchronization
|
|
|
|
@available(LambdaSwift 2.0, *)
|
|
extension LambdaHTTPServer {
|
|
|
|
/// A shared data structure to store the current invocation or response requests and the continuation objects.
|
|
/// This data structure is shared between instances of the HTTPHandler
|
|
/// (one instance to serve requests from the Lambda function and one instance to serve requests from the client invoking the lambda function).
|
|
internal final class Pool<T>: AsyncSequence, AsyncIteratorProtocol, Sendable where T: Sendable {
|
|
private let poolName: String
|
|
internal init(name: String = "Pool") { self.poolName = name }
|
|
|
|
typealias Element = T
|
|
|
|
struct State {
|
|
var buffer: Deque<T> = []
|
|
var waitingForAny: CheckedContinuation<T, any Error>?
|
|
var waitingForSpecific: [String: CheckedContinuation<T, any Error>] = [:]
|
|
}
|
|
|
|
private let lock = Mutex<State>(State())
|
|
|
|
/// enqueue an element, or give it back immediately to the iterator if it is waiting for an element
|
|
public func push(_ item: T) {
|
|
let continuationToResume = self.lock.withLock { state -> CheckedContinuation<T, any Error>? in
|
|
// First check if there's a waiting continuation that can handle this item
|
|
|
|
// Check for FIFO waiter first
|
|
if let continuation = state.waitingForAny {
|
|
state.waitingForAny = nil
|
|
return continuation
|
|
}
|
|
|
|
// Check for specific waiter
|
|
if let response = item as? LocalServerResponse,
|
|
let requestId = response.requestId,
|
|
let continuation = state.waitingForSpecific.removeValue(forKey: requestId)
|
|
{
|
|
return continuation
|
|
}
|
|
|
|
// No waiting continuation, add to buffer
|
|
state.buffer.append(item)
|
|
return nil
|
|
}
|
|
|
|
// Resume continuation outside the lock to prevent potential deadlocks
|
|
continuationToResume?.resume(returning: item)
|
|
}
|
|
|
|
/// Unified next() method that handles both FIFO and requestId-specific waiting
|
|
private func _next(for requestId: String?) async throws -> T {
|
|
// exit if the task is cancelled
|
|
guard !Task.isCancelled else {
|
|
throw CancellationError()
|
|
}
|
|
|
|
return try await withTaskCancellationHandler {
|
|
try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<T, any Error>) in
|
|
let nextAction: Result<T, PoolError>? = self.lock.withLock { state -> Result<T, PoolError>? in
|
|
if let requestId = requestId {
|
|
// Look for oldest (first) item for this requestId in buffer
|
|
if let index = state.buffer.firstIndex(where: { item in
|
|
if let response = item as? LocalServerResponse {
|
|
return response.requestId == requestId
|
|
}
|
|
return false
|
|
}) {
|
|
let item = state.buffer.remove(at: index)
|
|
return .success(item)
|
|
} else {
|
|
// Check for conflicting waiters
|
|
if state.waitingForAny != nil {
|
|
return .failure(PoolError(cause: .mixedWaitingModes))
|
|
}
|
|
if state.waitingForSpecific[requestId] != nil {
|
|
return .failure(PoolError(cause: .duplicateRequestIdWait(requestId)))
|
|
}
|
|
|
|
// No matching item, wait for it
|
|
state.waitingForSpecific[requestId] = continuation
|
|
return nil
|
|
}
|
|
} else {
|
|
// FIFO mode - take first item
|
|
if let first = state.buffer.popFirst() {
|
|
return .success(first)
|
|
} else {
|
|
// Check for conflicting waiters
|
|
if !state.waitingForSpecific.isEmpty {
|
|
return .failure(PoolError(cause: .mixedWaitingModes))
|
|
}
|
|
if state.waitingForAny != nil {
|
|
return .failure(PoolError(cause: .nextCalledTwice(state.waitingForAny!)))
|
|
}
|
|
|
|
state.waitingForAny = continuation
|
|
return nil
|
|
}
|
|
}
|
|
}
|
|
|
|
switch nextAction {
|
|
case .success(let item):
|
|
continuation.resume(returning: item)
|
|
case .failure(let error):
|
|
if case let .nextCalledTwice(prevContinuation) = error.cause {
|
|
prevContinuation.resume(throwing: error)
|
|
}
|
|
continuation.resume(throwing: error)
|
|
case .none:
|
|
// do nothing - continuation is stored in state
|
|
break
|
|
}
|
|
}
|
|
} onCancel: {
|
|
// Only remove THIS task's continuation
|
|
let continuationToCancel = self.lock.withLock { state -> CheckedContinuation<T, any Error>? in
|
|
if let requestId = requestId {
|
|
// Remove only the continuation for this specific requestId
|
|
return state.waitingForSpecific.removeValue(forKey: requestId)
|
|
} else {
|
|
// Remove only the FIFO continuation
|
|
let cont = state.waitingForAny
|
|
state.waitingForAny = nil
|
|
return cont
|
|
}
|
|
}
|
|
|
|
continuationToCancel?.resume(throwing: CancellationError())
|
|
}
|
|
}
|
|
|
|
/// Simple FIFO next() method - used by AsyncIteratorProtocol
|
|
func next() async throws -> T? {
|
|
try await _next(for: nil)
|
|
}
|
|
|
|
/// RequestId-specific next() method for LocalServerResponse - NOT part of AsyncIteratorProtocol
|
|
func next(for requestId: String) async throws -> T {
|
|
try await _next(for: requestId)
|
|
}
|
|
|
|
func makeAsyncIterator() -> Pool {
|
|
self
|
|
}
|
|
|
|
/// Cancel all waiting continuations - used during server shutdown
|
|
func cancelAll() {
|
|
let continuationsToCancel = self.lock.withLock { state -> [CheckedContinuation<T, any Error>] in
|
|
var toCancel: [CheckedContinuation<T, any Error>] = []
|
|
|
|
if let continuation = state.waitingForAny {
|
|
toCancel.append(continuation)
|
|
state.waitingForAny = nil
|
|
}
|
|
|
|
for continuation in state.waitingForSpecific.values {
|
|
toCancel.append(continuation)
|
|
}
|
|
state.waitingForSpecific.removeAll()
|
|
|
|
return toCancel
|
|
}
|
|
|
|
// Resume all continuations outside the lock
|
|
for continuation in continuationsToCancel {
|
|
continuation.resume(throwing: CancellationError())
|
|
}
|
|
}
|
|
|
|
struct PoolError: Error {
|
|
let cause: Cause
|
|
var message: String {
|
|
switch self.cause {
|
|
case .nextCalledTwice:
|
|
return "Concurrent invocations to next(). This is not allowed."
|
|
case .duplicateRequestIdWait(let requestId):
|
|
return "Already waiting for requestId: \(requestId)"
|
|
case .mixedWaitingModes:
|
|
return "Cannot mix FIFO waiting (next()) with specific waiting (next(for:))"
|
|
}
|
|
}
|
|
|
|
enum Cause {
|
|
case nextCalledTwice(CheckedContinuation<T, any Error>)
|
|
case duplicateRequestIdWait(String)
|
|
case mixedWaitingModes
|
|
}
|
|
}
|
|
}
|
|
}
|
|
#endif
|