swift-server/async-http-client

Add `HTTPClient.Configuration.browserLike`

weissi opened this issue · 7 comments

weissi commented

We should add a

extension HTTPClient.Configuration {
    public static var browserLike: Self {
        ...
    }
}

which should return an evolving configuration that's always as close as possible to the platform's default/dominant browser.

Yes, AHC can't do everything that a browser can, but we can get as close as possible.

weissi commented

@Lukasa Would you be able to suggest a Configuration that is the most "browser like"?

weissi commented

Something like

        HTTPClient.Configuration(
            tlsConfiguration: nil             /* default */,
            redirectConfiguration:            /* enabled but with sensible limits (max 10 redirects or so) */,
            timeout:                          /* set, similar to browsers */,
            connectionPool: ConnectionPool()  /* enabled, maximum connections similar to browsers */,
            proxy:                            /* depending on the http_proxy, HTTP_PROXY, HTTPS_PROXY, ... env vars */,
            ignoreUncleanSSLShutdown: false,
            decompression:                    /* enabled with sensible limits */
        )
Lukasa commented

The above is broadly right, though ignoreUncleanSSLShutdown would typically be true.

weissi commented

Thank you for looking into this.

The above is broadly right, though ignoreUncleanSSLShutdown would typically be true.

@Lukasa This option is a kinda a legacy option. AHC now does a really good job at handling unclean shutdowns: If we have chunked encoding or a content length we only complain if it was truly truncated. IIRC, the only way if it was actually truncated (and we should and maybe do send a different error like truncatedResonse or something for that) or if EOF framing is in use over TLS.

Are you saying that browsers don't complain at all on EOF-framing over TLS without CloseNotify?

Lukasa commented

Oh yeah, sorry, I forgot we changed the behaviour. I actually don't know, but you're right that if we only alert here in cases where there is actually a truncation risk then we should set this to false.

This should be a static var right?

weissi commented

This should be a static var right?

Whoops yes, I will fix by editing!