HTTPClientRequest: allow custom TLS config

[weissi]

In the transition from HTTPClient.Request to HTTPClientRequest we unfortunately lost some important functionality: Custom TLS configuration.

The can be important to override on a per-request basis but more importantly it was the most significant step which allows us to unlock the 3-tier architecture (#392). For the 3-tier architecture it's very important to get rid of any top-level configuration.

This PR brings back the lost functionality.

[fabianfett]

Love it! I should have done this way earlier! I especially like that we add this property to the request itself and don't pass it as a function parameter!

[dnadoba]

This doesn't belongs to the HTTPRequest. It is extra configuration and belongs into a separate type.

This belongs into tier 2 of #392.

[weissi]

This doesn't belongs to the HTTPRequest. It is extra configuration and belongs into a separate type.

This belongs into tier 2 of #392.

Hang on, tier 3 can override anything from tier 2.

The idea is:

• tier 1 (the connection pool/TLS context cache) has (next to) no configuration
• tier 2 is a holder of configuration you need a lot
• tier 3 is the request and may override tier 2 for the odd property

For example (hypothetical API):

// HTTPService is tier 1

var client = HTTPClient(service: HTTPService.shared) // HTTPClient is tier-2
client.redirects = .max(5)
client.tlsConfiguration = mySuperSpecialTLSConfig

var apiRequest = HTTPClientRequest() // HTTPClientRequest is tier-3
apiRequest.redirects = .disallow // API request doesn't need redirects
apiRequest.url = "https://..."
try await client.execute(apiRequest)

var webRequest = HTTPClientRequest()
webRequest.url = "https://..."
try await client.execute(webRequest)

[dnadoba]

As discussed in #392, there seems to be no point in tier 3 if tier 2 is ~free to create.

[weissi]

As discussed in #392, there seems to be no point in tier 3 if tier 2 is ~free to create.

Convenience and you don't need to repeat everything.

For example, I might want to ~always set a certain header in my tier 2 but in tier 3 I definitely want to add headers.

Like

actor FooClient {
    private let client: HTTPClient = {
        var client = HTTPClient(...)
        client.headers["user-agent"] = "my super lib/123"
        [...]
        return client
    }

    func upgradeFoo() {
        var request = HTTPClientRequest()
        request.url = "..."
        request.headers["foo-level"] = "max"
        try await self.client.execute(request)
    }
}

[dnadoba]

I agree with that, default headers on tier 2 are great. What I don't agree with is that tier 3 has override for everything in tier 2. Tier 3 should just be (HTTPRequest, Body) where HTTPRequest is the new type form swift-http-types and Body is some kind of AsyncSequence. Nothing else.

[weissi]

I agree with that, default headers on tier 2 are great. What I don't agree with is that tier 3 has override for everything in tier 2. Tier 3 should just be (HTTPRequest, Body) where HTTPRequest is the new type form swift-http-types and Body is some kind of AsyncSequence. Nothing else.

I think that's important. But also irrelevant for this PR. This PR brings back accidentally lost functionality when we added HTTPClientRequest a close friend of HTTPClient.Request (even documented that way). HTTPClient.Request has tlsConfiguration and so should HTTPClientRequest.

In the future when we pick up swift-http-types the whole nomenclature of what a "request" even is will change. Then we can discuss where the various configuration options can go. But this is the wrong place IMHO, we're just adding stuff back that we forgot before.

[dnadoba]

After thinking about this again I came to the conclusion that it doesn't hurt to add this before we implement the 3-tier architecture as we will likely deprecate HTTPClientRequest anyway.

Minor nits, afterwards we are good to go from my side.

[dnadoba]

lets get ride of the force unwrap here as well.

[weissi]

but that would hide issues. Force unwraps here are good because they'd catch that going wrong at the right place here

[dnadoba]

No, nil is the default configuration and it would be reasonable to normalise the input value to become nil after setting it to the default value.

Note that this shouldn't be an Optional property to begin with but that's the current API.

[weissi]

No, that would not be reasonable, that would be user hostile. If this breaks, AHC is broken, let's not hide this

[dnadoba]

that's a bit long

[weissi]

disagree. Tests with timeouts that aren't ~forever really suck because at some point CI is slow and then you'll raise the timeout again. So you never quite know why it failed: Was CI slow or did it actually time out. If we set it to ~never (hours(99)) then if it times out, it'll be killed by CI after the long CI timeout and we know that this is a true failure that needs to be fixed.

That means you'll have one timeout (the CI timeout) that is actually meaningful if it's breached. If everybody puts like .seconds(1) or .seconds(10) all over their tests then it requires constant thinking which leads to everybody just ignoring and restarting failed tests.

[weissi]

I wanted to use .hours(.max) to make this clear but apple/swift-nio#2532

[dnadoba]

But this doesn't really help with unreasonably slow CIs as there are more timeouts with a default value. Also tests don't only run only CI but also locally and in compatibly suites. A reasonable time limit of 30 seconds is appropriate to be a good citizen if run in a compatibility suite.

[weissi]

Any system that runs tests needs to already deal with tests taking forever. For example if you hit a deadlock. We shouldn't introduce another category of failures.

[weissi]

@dnadoba can we get this over the line please?

[dnadoba]

@swift-server-bot test this please