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

[HTTP2] Make HTTPVersion public and set to .automatic by default #473

Merged
merged 4 commits into from
Nov 11, 2021

Conversation

dnadoba
Copy link
Collaborator

@dnadoba dnadoba commented Nov 10, 2021

This PR does two things:

  • makes it possible to enabling and disabling HTTP/2 by making HTTPVersion public
  • set HTTPVersion to .automatic by default

HTTP/2 will then be used if we connect to a server with HTTPS and the server supports HTTP/2, otherwise we will continue to use HTTP/1.

A user will still be able to disable HTTP/2 if they want to by setting HTTPVersion to .http1Only on HTTPClient.Configuration e.g.:

var configuration = HTTPClient.Configuration()
configuration.httpVersion = .http1Only
let client = HTTPClient(
    eventLoopGroupProvider: .createNew,
    configuration: configuration
)

)
}

// TODO: make public
internal init(
public init(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH: I don't think we should add yet another initializer for HTTPClient.Configuration. I think we should deprecate all of them, except for init(). In init() we set all default values and then the user can change the config, since it is just a struct. We have precedent for this move in NIOSSL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand you correct, a user will then only be able to change the config by create a temporary variable, initialising it with our default config, mutating it and passing it to the HTTPClient initialiser, right? This would be a rather big change because we would need to update all our tests if we deprecate all other inits. I would rather do that in another PR if we would like to do that before merging this one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Lukasa

Copy link
Collaborator Author

@dnadoba dnadoba Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also think about adding an extension to HTTPClient.Configuration so we can change the configuration through daisy chaining like I have added in RSocket:

extension HTTPClient.Configuration {
    /// Creates a copy and updates the property described by `keyPath` with the given `newValue`
    /// - Returns: updated configuration
    public func set<Value>(_ keyPath: WritableKeyPath<Self, Value>, to newValue: Value) -> Self {
        var copy = self
        copy[keyPath: keyPath] = newValue
        return copy
    }
}

This would ease the transition because you probably end up with the same amount of lines as with an init and a user does not have to add a temporary variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we are already on it, what about nested types HTTPClient.Configuration uses e.g. HTTPClient.Configuration.ConnectionPool and HTTPClient.Configuration.Timeout? They will probably suffer from the same problem, but on a much smaller scale.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to tackle this in a separate PR. For now we don't need to add a new init for this feature, we can just exclude it from the pre-existing inits.

@fabianfett
Copy link
Member

Fixes #28.

@dnadoba dnadoba force-pushed the dn-http2-enable-by-default branch 2 times, most recently from a061d78 to 92444bd Compare November 11, 2021 10:13
@fabianfett fabianfett added this to the HTTP/2 support milestone Nov 11, 2021
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It's great that we can finally expose this!

@dnadoba dnadoba force-pushed the dn-http2-enable-by-default branch from 458bf4b to 5bf0bf6 Compare November 11, 2021 11:03
@dnadoba dnadoba marked this pull request as ready for review November 11, 2021 11:10
@fabianfett fabianfett requested a review from Lukasa November 11, 2021 12:23
@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Nov 11, 2021
@dnadoba dnadoba merged commit 3e44e51 into swift-server:main Nov 11, 2021
@fabianfett fabianfett mentioned this pull request Nov 11, 2021
@dnadoba dnadoba deleted the dn-http2-enable-by-default branch November 11, 2021 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants