Allow injection of externally initialized ByteBufferAllocator

[Buratti]

Allow the runtime's ByteBufferAllocator to be shared with other systems which are initialized before the handler is instantiated.

Motivation:

LambdaRuntimeFactory.makeRuntime is intended for scenarios requiring high customizability. It's reasonable to assume that these scenarios typically involve other subsystems, which might be initialized before LambdaRuntimeHandler.init(context:).

The simplest example is a structured swift-log LogHandler, where logs are formatted and encoded into ByteBuffers. In this case, LoggingSystem.bootstrap needs to run before LambdaRuntimeFactory.makeRuntime, directly inside the @main entry point, and not inside the LambdaRuntimeHandler.init(context:) initializer.

Modifications:

The private allocator of LambdaRunner is now injected through its initializer. All the makeRuntime methods take a ByteBufferAllocator parameter which defaults to a freshly initialized one if left unspecified. This prevents any breaking changes.

Result:

static func main() async throws {
    let sharedAllocator = ByteBufferAllocator()
    LoggingSystem.bootstrap { label in
        CodableLogHandler(label: label, allocator: sharedAllocator)
    }
    let logger = Logger(label: "Lambda")
    
    // ...
    
    let runtime = LambdaRuntimeFactory.makeRuntime(
        handlerProvider: { context in
            eventLoop.makeSucceededFuture(
                SomeLambdaHandler(context: context)
            )
        },
        eventLoop: eventLoop,
        allocator: sharedAllocator,
        logger: logger
    )
    // ...
}

[sebsto]

@swift-server-bot test this please

[sebsto]

@fabianfett wdyt ?

[sebsto]

@swift-server-bot test this please

[sebsto]

@adam-fowler if time permits, can you have a look at this PR please ?

[fabianfett]

I'm inclined to not support this change. For a couple of reasons:

1. This makes the API surface more complicated. I'm happy to increase the API surface, if we can get a measurable benefit from this. So what I would like to see is a measurements that shows how much of a performance gain the shared ByteBufferAllocator provides. Given that ByteBufferAllocator currently does not have some magic buffer reuse tactics, I doubt that this will yield any benefit.
2. This would introduce another NIO object into our public API. In the future we want to make NIO an implementation detail and I'm afraid that this change moves this into the wrong direction.

I want to point out that the correct NIO pattern here would be to forward the ByteBufferAllocator from the channel's ChannelContext to the user and not one that got allocated once.

No doubt is the current API weird and we are looking into improving it.

[Buratti]

I don't see how this change would negatively impact the API surface. The LambdaRuntime class isn't meant for standard development (where you usually interact with a *LambdaHandler type), it is offered as part of the public API as a way to develop high-level frameworks on top of the runtime. As such, I would argue that the more "configurable" this type is, the better it becomes when it comes to develop a framework on top of it.

It is true that there isn't any performance benefit from this yet. Nonetheless, NIO's best practices suggest to reuse the allocators as much as possible, as the "magic buffer reuse" should be available eventually.

I wasn't aware of the fact that NIO is meant to become an internal import in the future, in such case, I agree with not supporting this change.

[sebsto]

Despite the fact we will continue to expose ByteBuffer in v2, we will close this PR as per the discussion above with @fabianfett