Fixes for Local Lambda Server #486
Conversation
| } | ||
|
|
||
| case .continuation: | ||
| fatalError("Concurrent invocations to next(). This is illigal.") |
| @@ -350,44 +404,67 @@ private struct LambdaHttpServer { | |||
| private final class Pool<T>: AsyncSequence, AsyncIteratorProtocol, Sendable where T: Sendable { | |||
| typealias Element = T | |||
|
|
|||
| private let _buffer = Mutex<CircularBuffer<T>>(.init()) | |||
| private let _continuation = Mutex<CheckedContinuation<T, any Error>?>(nil) | |||
| struct State { | |||
There was a problem hiding this comment.
Do we really need a struct here ? Can't we simplify and just define the enum ?
Mutex's T must be Copyable, therefore passing an enum is accepted.
enum Test {
case A
case B
}
private let m = Mutex<Test>(.A)
There was a problem hiding this comment.
Reduced it down to an enum but marked the enum as ~Copyable to ensure we don't allocate when we hold the lock.
| return result | ||
|
|
||
| case .serverReturned: | ||
| fatalError() |
There was a problem hiding this comment.
Should we add a message to the fatalError : "Only one task is a server, and only one can return serverReturned"
Also what about renaming task1 and task2 with serverOrHandlerResult1 and serverOrHandlerResult2
| @@ -13,6 +13,7 @@ | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #if DEBUG | |||
| import DequeModule | |||
| import Dispatch | |||
| import Logging | |||
| import NIOConcurrencyHelpers | |||
There was a problem hiding this comment.
Do we still need this import now that we don't use CircularBuffer anymore ?
| private let group: EventLoopGroup | ||
| private let host: String | ||
| private let port: Int | ||
| private struct LambdaHTTPServer { |
There was a problem hiding this comment.
Should we update the name of this struct ? This is not the HTTP Server anymore (all the NIO bootstrap and channel creation is done in the withLocalServer static function)
This struct now just provides the handleConnection() function
But I can't think about a descriptive name :-)
There was a problem hiding this comment.
Lets keep it for now. We can always change it later. It's internal.
| return .serverReturned(.failure(error)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think 2 lines of comments to explain the logic below wouldn't hurt readability :-)
// Now that the local HTTP server and LambdaHandler tasks are started, wait for the first of the two that will terminate.
// When the first task terminates, cancel the group and collect the result of the second task
| if let element = await self.popFirst() { | ||
| return element | ||
| } else { | ||
| // we can't return nil if there is nothing to dequeue otherwise the async for loop will stop |
There was a problem hiding this comment.
I would keep those two comments. They explain why this is not a regular async iterator. It blocks when the buffer is empty
| group.cancelAll() | ||
| let task2 = await group.next()! | ||
|
|
||
| switch task1 { |
There was a problem hiding this comment.
// collect and return the result of the LambdaHandler
| case .closureResult(let result): | ||
| return result | ||
|
|
||
| case .serverReturned: |
There was a problem hiding this comment.
should we log.error() the fact that the server terminated before the Handler? It might be an error in the server implementation or an otherwise important information to show to the user
|
@fabianfett Over the next week-end, I will add a unit test suite for the pool implementtaion. |
|
@fabianfett there is also a minor format check on |
fabianfett
added
🔨 semver/patch
No description provided.