eventLoopGroupProvider provider for async http client

[thoven87]

Thanks for much for putting this great library together. I have been testing it for a few days now.

I come to the conclusion that eventLoopGroupProvider should not be a required parameter since it's only used the async-http-client. Should WebPushManager take in an instance of the http client instead? In Hummingbird one can create a single HTTPClientService that gets shared amongst all libraries that needs an instance of the http client.

Please let me know if this is a welcome change as it is also a breaking change. I can a PR out if needed.

I think this deinit can be replaced with the service lifecycle. An example of the service life cycle can be seen here we would simply add WebPushManager as a service graceful shutdown would be handle cleanly

https://github.com/swift-server/swift-service-lifecycle

[dimitribouniol]

Just because Hummingbird provides an HTTPClient, does not mean it should be shared by the WebPushManager — it may have a very different set of expectations than the one provided after all (namely HTTP/2 support, no TLS client overrides, etc…). Not to mention, pipelining will likely be tweaked in the near future, so a shared HTTP client may likely interrupt this process from working smoothly.

The purpose of eventLoopGroupProvider is to share the actual resource that needs to be shared throughout the process: the NIO event loop group. By default, the shared event loop group is used, so the parameter is not even required as a result, but if you did customize it, you can ask your Hummingbird Application's eventloopGroup for a value to use:

let manager = WebPushManager(
    vapedConfiguration: config,
    backgroundActivityLogger: logger,
    eventLoopGroupProvider: .shared(application.eventloopGroup)
)

The purpose of the deinit is to facilitate usage when swift-service-lifecycle is not used (mostly because there was a ton of boilerplate in tests, and in existing Vapor apps). WebPushManager is in fact a compatible Service that can be installed alongside the rest of the ones you use.

If you run into any integration issues with what I mentioned above, please let me know!

[thoven87]

Just because Hummingbird provides an HTTPClient, does not mean it should be shared by the WebPushManager — it may have a very different set of expectations than the one provided after all (namely HTTP/2 support, no TLS client overrides, etc…). Not to mention, pipelining will likely be tweaked in the near future, so a shared HTTP client may likely interrupt this process from working smoothly.

The purpose of eventLoopGroupProvider is to share the actual resource that needs to be shared throughout the process: the NIO event loop group. By default, the shared event loop group is used, so the parameter is not even required as a result, but if you did customize it, you can ask your Hummingbird Application's eventloopGroup for a value to use:

let manager = WebPushManager(
    vapedConfiguration: config,
    backgroundActivityLogger: logger,
    eventLoopGroupProvider: .shared(application.eventloopGroup)
)

The purpose of the deinit is to facilitate usage when swift-service-lifecycle is not used (mostly because there was a ton of boilerplate in tests, and in existing Vapor apps). WebPushManager is in fact a compatible Service that can be installed alongside the rest of the ones you use.

If you run into any integration issues with what I mentioned above, please let me know!

Thanks for such a detail response. Hummingbird does not provide an HTTPClient by default. It makes use of the swift service life cycle. Unless, the HTTPClient's configuration needs changes inside WebPushManager which I don't think it does today. An instance of the Client should be passed instead. I don't any strong opinions of requiring an eventLoopGroupProvider provider or not. Would provide another init definition which takes in an instance of HTTPClient.shared suffice?

How does one handle graceful shutdown with the current implementation? Since the eventLoopGroupProvider is not even required?

[dimitribouniol]

Hummingbird does not provide an HTTPClient by default. It makes use of the swift service life cycle. Unless, the HTTPClient's configuration needs changes inside WebPushManager which I don't think it does today. An instance of the Client should be passed instead. I don't any strong opinions of requiring an eventLoopGroupProvider provider or not. Would provide another init definition which takes in an instance of HTTPClient.shared suffice?

The key piece here is that the HTTPClient itself should not be pre-configured in any special way, as that'll likely mess with functionality in a non-useful way — HTTPClients don't even need to be shared, you really can make as many of them as you want with different configurations and they'll play together just fine, because again, they share the event loop group under the hood. You really should be interpreting the HTTPClient as an implementation detail, not a hot-swappable component.

If your goal for passing an HTTPClient is to mock it in tests, the WebPushTesting module provides a much more convenient .mockedManager { message, subscriber, expiration, urgency in ... } initializer you can pass to your controllers for making sure notifications are sent or are otherwise handling errors correctly.

How does one handle graceful shutdown with the current implementation? Since the eventLoopGroupProvider is not even required?

Something like this should work just fine, if you are already using swift-service-lifecycle:

let manager = WebPushManager(
    vapidConfiguration: vapidConfig,
    backgroundActivityLogger: logger
)
try await ServiceGroup(
    services: [
        manager
    ],
    logger: logger
).run()

[dimitribouniol]

There is a new initializer that takes an unsafeHTTPClient: if you really want this, but generally you shouldn't need it. Feel free to re-open this or a new issue if that's not enough!