Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document the suggested way to get an EventLoopGroup #2498

Open
weissi opened this issue Aug 9, 2023 · 13 comments
Open

Document the suggested way to get an EventLoopGroup #2498

weissi opened this issue Aug 9, 2023 · 13 comments
Labels
discussion for things that need discussion enhancement New feature or request needs-no-version-bump For PRs that when merged do not need a bump in version number.

Comments

@weissi
Copy link
Member

weissi commented Aug 9, 2023

In the past, NIO and other libraries carried the flawed (#2142) *EventLoopGroupProvider pattern. There were two primary concerns:

  1. The easiest way to use other libraries was wasteful (.createNew)
  2. Shutdown was ~impossible to do nicely because you can't return a future from an EventLoopGroup that you own on shutdown, because you just shut it down... Also it's weird to sometimes own the group...

Now, with the singleton ELGs (#2471) the pattern should evolve into something more useful.

I think @fabianfett and my current thinking looks like this (but this is of course all open for debate): Let's assume we're building FooClient, a client library for the Foo service.

It might have a top-level

public final class/actor FooClient {
    public init(eventLoopGroup: any EventLoopGroup = FooClient.defaultEventLoopGroup)

    public static var defaultEventLoopGroup: EventLoopGroup {
#if canImport(Network)
        if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
            return NIOTSEventLoopGroup.singleton
        } else {
            return MultiThreadedEventLoopGroup.singleton
        }
#else
        return MultiThreadedEventLoopGroup.singleton
#endif
    }
}

Which gives us all the benefits:

  • FooClient never owns the EventLoopGroup -> shutdown's easy
  • FooClient is easy to create (FooClient(), done)
  • If you have an EventLoopGroup you can easily have FooClient use it too: FooClient(eventLoopGroup: myBetterELG)
  • FooClient can support alternative EventLoopGroups like the ones in NIOTS.
  • FooClient can start out only supporting NIOPosix and later add NIOTS without API breaks

Possible extension: If FooClient still owns non-memory resources (like a connection pool) that cannot be shut down synchronously, it's worth to consider adding a singleton-FooClient instance too:

extension FooClient {
    public static var shared: FooClient {
        return globalFooClientSingleton
    }
}

private let globalFooClientSingleton: FooClient = {
    return FooClient()
}()

The biggest risk here is configuration: This will work well if FooClient itself takes no configuration, this will be more troublesome if FooClient does have configuration: What config should the singleton have?

Relevant discussion/information:


Alternative: A more lightweight alternative might be

public final class/actor FooClient {
    private let eventLoopGroup: any EventLoopGroup

    // nil means "default ELG"
    public init(eventLoopGroup: any EventLoopGroup? = nil)
        self.eventLoopGroup = eventLoopGroup ?? Self.defaultEventLoopGroup
    }

    // note, this is internal here
    internal static var defaultEventLoopGroup: EventLoopGroup {
#if canImport(Network)
        if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
            return NIOTSEventLoopGroup.singleton
        } else {
            return MultiThreadedEventLoopGroup.singleton
        }
#else
        return MultiThreadedEventLoopGroup.singleton
#endif
    }
}

I think I prefer the first suggestion FooClient(eventLoopGroup: FooClient.defaultEventLoopGroup) because it this might read like it's not using an ELG: FooClient(eventLoopGroup: .none)/FooClient(eventLoopGroup: nil).


Open questions:

  • Naming: Is defaultEventLoopGroup really appropriate for "the platform-default globally shared singleton group"?
@FranzBusch
Copy link
Contributor

Thanks for opening this issue. This is definitely something we need to provide guidance on. I am a bit worried about this code:

    internal static var defaultEventLoopGroup: EventLoopGroup {
#if canImport(Network)
        if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
            return NIOTSEventLoopGroup.singleton
        } else {
            return MultiThreadedEventLoopGroup.singleton
        }
#else
        return MultiThreadedEventLoopGroup.singleton
#endif
    }

Since it defaults to the ELG depending on the platform; however, we have quite a few use-cases where want to pick POSIX on even Darwin platforms. My current thinking was along the lines of creating a new package (let's call it swift-nio-universal-bootstrap) that solves the problem of providing a universal cross platform configuration for bootstrapping a TCP/UDP client & server. This should now be possible including TLS configuration with the new swift-certificates library.

I haven't fully figured out how this should look but my thinking was something along those lines (all pseudocode):

struct TCPClientBootstrapConfiguration {
    struct POSIX {
        var eventLoopGroup: any EventLoopGroup // Defaults to the singleton group
    }
    
    struct NetworkFramework {
        var eventLoopGroup: any EventLoopGroup // Defaults to the singleton group
    }

    static func automatic(posixConfiguration: POSIX, networkFrameworkConfiguration: NetworkFramework) -> Self
    // This would throw/fatalError if run on the wrong platform
    static func forcePosix(posixConfiguration: POSIX) -> Self
    // This would throw/fatalError if run on the wrong platform
    static func forceNetworkFramework(posixConfiguration: POSIX) -> Self
}

Then concrete protocol client/server libraries should be able to take this configuration as part of their larger configuration and they just pass that through to the underlying universal bootstrap APIs.

This should make it a lot easier for client/server library authors to become Multiplatform by default and easily exposing the configuration to the end-user of their library. They would then just declare their client like this

final class FooClient {
    struct Configuration {
         var tcpClientConfiguration: TCPClientBootstrapConfiguration = .automatic
     }
     
     init(configuration: Configuration)
}

@Lukasa
Copy link
Contributor

Lukasa commented Aug 9, 2023

swift-certificates does not solve the problem of TLS configuration, as certificates provided by swift-certificates cannot easily be mapped into a SecIdentity.

@FranzBusch
Copy link
Contributor

swift-certificates does not solve the problem of TLS configuration, as certificates provided by swift-certificates cannot easily be mapped into a SecIdentity.

Oh then I misunderstood our last conversation around this. I thought that is possible.

@weissi
Copy link
Member Author

weissi commented Aug 9, 2023

Here's the AHC PR that implements something along those lines: swift-server/async-http-client#697

@weissi
Copy link
Member Author

weissi commented Aug 9, 2023

@FranzBusch An env var override is probably necessary. If the programmer wants to override, that's easy: FooClient(eventLoopGroup: MultiThreadedEventLoopGroup.singleton) forces POSIX and also sorts out the TLS (because AHC figures out what ELG implementation is in use and then matches the required TLS implementation: https://github.com/swift-server/async-http-client/blob/e1c85a65d82a5fcd49e74e6afb80362d058ca066/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool%2BFactory.swift#L411)

@FranzBusch
Copy link
Contributor

I know that AHC does this dance but it is really a pity that every single server/client implementation has to do the same dance again. I would really love if we can find a single abstraction for this that makes this easier for the ecosystem.

@weissi
Copy link
Member Author

weissi commented Aug 9, 2023

I know that AHC does this dance but it is really a pity that every single server/client implementation has to do the same dance again. I would really love if we can find a single abstraction for this that makes this easier for the ecosystem.

Yes, it's painful (as we expect everybody to copy https://github.com/apple/swift-nio-examples/tree/main/UniversalBootstrapDemo). I have previously advocated for a swift-nio-family repository that depends on "everything" and allows you to create the correct TLS + EventLoopGroup configuration pairs (leveraging the universal bootstrapping mechanism to do so).

Apart from a swift-nio-family (or similar) repo, the only other option would be something in swift-nio where other packages can be "registered". Like a global NIOTransportServices.register() which would then add itself to a global table of supported configurations that the universal bootstrap could then query. In theory this registration could also be done via the linker with what C calls __attribute__((constructor)) functions (i.e. code that runs at startup without the user doing anything). This automatic stuff didn't feel clean so we never did that...

@weissi
Copy link
Member Author

weissi commented Aug 9, 2023

But to be clear: This is a related issue and is not the core that's to be discussed here. Whatever we do, every package will likely want a way to get a "default singleton ELG".

Sure, it'd be nice if we didn't have to copy & paste code but that can be fixed at a later point in time. That's not blocking the more urgent issue of getting rid of EventLoopGroupProviders.

@FranzBusch
Copy link
Contributor

But to be clear: This is a related issue and is not the core that's to be discussed here. Whatever we do, every package will likely want a way to get a "default singleton ELG".

Fair enough. I think overall I would vote for this approach

    public init(eventLoopGroup: any EventLoopGroup? = nil)
        self.eventLoopGroup = eventLoopGroup ?? Self.defaultEventLoopGroup
    }

It avoids baking the default ELG into any API and we can solve the nil = default via documentation.

@weissi
Copy link
Member Author

weissi commented Aug 9, 2023

It avoids baking the default ELG into any API

Just to be clear, any of the suggestions here avoids this. eventLoopGroup: any EventLoopGroup = FooClient.defaultEventLoopGroup also doesn't bake the ELG type into API at all. It's completely opaque.

@FranzBusch
Copy link
Contributor

Just to be clear, any of the suggestions here avoids this. eventLoopGroup: any EventLoopGroup = FooClient.defaultEventLoopGroup also doesn't bake the ELG type into API at all. It's completely opaque.

I phrased this a bit badly. I don't see a motivating reason to expose a FooClient.defaultEventLoopGroup as part of the public API of FooClient if the only reason is to use it as a default parameter. I prefer to use nil and let us fallback internally. Somebody might use the FooClient.defaultEventLoopGroup then for whatever reason which is not necessary at all.

@weissi
Copy link
Member Author

weissi commented Aug 9, 2023

Just to be clear, any of the suggestions here avoids this. eventLoopGroup: any EventLoopGroup = FooClient.defaultEventLoopGroup also doesn't bake the ELG type into API at all. It's completely opaque.

I phrased this a bit badly. I don't see a motivating reason to expose a FooClient.defaultEventLoopGroup as part of the public API of FooClient if the only reason is to use it as a default parameter. I prefer to use nil and let us fallback internally. Somebody might use the FooClient.defaultEventLoopGroup then for whatever reason which is not necessary at all.

Okay, I'm exactly the other way around. I've seen too many APIs like

Configuration(redirectsAllowed: .none)

where you'd read "ahh, no redirects allowed" and in fact it means "default config" for redirectsAllowed, e.g. 5 redirects allowed. In certain cases it's even security-relevant.

Using nil / .none is often just unnecessarily confusing and I find giving a .default is just much better style (and not much more typing either).

Therefore -- despite it not being dangerously confusing here -- I think we should go for .default...

@jonct
Copy link

jonct commented Aug 14, 2023

Please pardon my barging in. Might the phrasing issue be resolved in the function signature, rather than with an enum?
e.g.
public init(customEventLoopGroup: any EventLoopGroup? = nil)
or
public init(overriding eventLoopGroup: any EventLoopGroup? = nil)

@weissi weissi added enhancement New feature or request discussion for things that need discussion needs-no-version-bump For PRs that when merged do not need a bump in version number. labels Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion for things that need discussion enhancement New feature or request needs-no-version-bump For PRs that when merged do not need a bump in version number.
Projects
None yet
Development

No branches or pull requests

4 participants