swift-server/async-http-client

HTTPClient.shared (like URLSession.shared)?

weissi opened this issue · 10 comments

weissi commented 
private let sharedSingleton: HTTPClient = {
    let httpClient = HTTPClient(eventLoopGroupProvider: .singleton)
    Unmanaged.takeUnretained(httpClient) // don't ever deinit this guy
}()

extension HTTPClient {
    public var shared: HTTPClient {
        return sharedSingleton
    }
}

? This has some issues around the configuration (solved by #392) but I'm pretty sure we can just create a somewhat sensible configuration that is standard and call it a day. If users do need special config, they can always create their own HTTPClient(eventLoopGroupProvider: .singleton, configuration: ...).

Possible once #697 is merged

weissi commented

I think the configuration should be .browserLike, see #701

weissi commented

@Lukasa / @dnadoba / @fabianfett What do we think, is more singletons acceptable? I hate to say it but I think we should do it for AHC too :|

fabianfett commented

@weissi Not sure here because this will introduce tons of discussion how this default AHC should be configured. Once we have the three later design this can be another question. For now I’m against it, mostly because of the discussions it will undoubtedly bring.

fabianfett commented

One other question...

As we try to move more and more code to structured concurrency, we basically always need a run() method that is the root of all background work. Would we spawn an unstructured Task for this, when users access the singleton for the first time?

weissi commented

Not sure here because this will introduce tons of discussion how this default AHC should be configured.

That's exactly why I'm suggesting .browserLike making it clear that when there's a discussion we'll try to match the default/prevalent browser.

As we try to move more and more code to structured concurrency, we basically always need a run() method that is the root of all background work. Would we spawn an unstructured Task for this, when users access the singleton for the first time?

There are two things in here:

  • Should AHC provide a structured run() method? Yes, different issue though.
  • Does this affect the singletons? No. Singletons are the easiest bits under structured concurrency: They never terminate so they can do whatever in the background and they remain structured. Conceptually, singletons are "around" the main function, so their scope never ends, i.e. they never finish (and on top of that they can't be cancelled). Do I like that? No. But the Apple SDKs work like that and it's "easy"...

For better or worse, Structured Concurrency is actually a strong argument in favour of the singletons: Many simple functions will need to do HTTP calls. So you have three options:

  1. Singletons, easy, just use that
  2. Single function that starts & stops the HTTP client in itself: Remains structured but wasteful and slow (connection pool gets stopped after 1 request)
  3. Make it a full-blown thing with a run() and a with method: Works, can be fast but is really annoying to use. A simple thing like suddenly becomes a whole ceremony.

There's absolutely zero change most developers will do (3). So they'll choose between singletons (1) and start+stop inline (2). The singletons are easier to use and much more performant, so sadly I think that's what we have to do.

FranzBusch commented

I am a bit torn on this. I do see the usefulness of just having an AsyncHTTPClient.browserLike singleton available but I am not too jazzed about bringing the whole Darwin singleton pattern over into the server ecosystem. I think for ELs it was the right thing to do since they often just live for the whole process duration and we can now treat them similarly to how the Concurrency thread pool behaves.

When it comes to clients I am personally leaning more towards having concrete initialisers and letting people pass them around. The main motivation for me here is to nudge people towards dependency injection and hopefully abstracting away the client behind protocols so that they can write proper unit tests. Furthermore, if I expect a bunch of libraries to start exposing a run() method for their clients (this doesn't require withXXX style API at all). So I don't think having the HTTPClient being special and providing a singleton is coherent with what other libraries are going to. Most libraries can't offer the singleton experience because they require explicit configuration on initialisation.

With the new service-lifecycle APIs, hopefully, people are just creating all their clients/server on application start and just put them into the ServiceGroup. Adding an HTTPClient to that group isn't hard and then they can just pass it down.

weissi commented

@FranzBusch I totally sympathise with you and I agree in principle. However, passing them around explicitly (which is the right thing to do) has API implications for anything between main and your library.

Especially given the existence of URLSession, people will always complain that they're supposed to add some dependency injection solution just because some tiny method somewhere might use an HTTPClient.

To sum it up, these are the reasons why I think we should offer the singleton:

  • URLSession exists, it's hard for us to explain why AHC can't be as convenient as URLSession
  • HTTP is so ubiquitous that it's not always obvious when you design your API that you may need to ask for an HTTP client to be injected later
  • There is no generally-accepted dependency injection library for Swift that would make it easier to declare that you suddenly need an HTTP client now
FranzBusch commented

I have been thinking about this a bit more since the Darwin argument wasn't convincing me fully I looked into other server ecosystems and how they tackle this.

  • Go: Offers the http package which allows one to just write http.Get to make requests. At the same time one can create a customised client if one wants to change tls and co.
  • Rust: Has a bunch of different options but I focused on reqwest. It follows the same pattern as Go an allows to do reqwest::get.

This and your example usage in Soto has convinced me that we should do this. I do think we should bike shed a bit on the name and maybe want to expose something like AsyncHTTPClient.get() instead of AsyncHTTPClient.browserLike.get() but overall I am convinced we should do this.

On the structured Concurrency note, I think this is okay. IMO, for such things we can spawn a top level detached task that runs the connection pool and any other things that need to be tied to a task. As long as we do the same lazy initialisation as @weissi did in the singleton ELs that should all be fine.

weissi commented

This and your example usage in Soto has convinced me that we should do this. I do think we should bike shed a bit on the name and maybe want to expose something like AsyncHTTPClient.get() instead of AsyncHTTPClient.browserLike.get()

I'd say let's follow the Apple SDK guidelines, HTTPClient is definitely a "user type" that regular developers who don't want to (or should have to) learn about SwiftNIO want to use on a daily basis. And therefore I proposed

let example = try await HTTPClient.shared.get("https://example.com")

much like URLSession.shared.... The .browserLike is just the name for the HTTPClient.Configuration that HTTPClient.shared singleton is initialised with. So we would also provide a HTTPClient.Configuration.shared singleton config which is like .default but much like a browser, so its values aren't API, the API is that it behaves like a browser.