Skip to content

Add HTTPConnectionPool Connection as a box type #398

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

Merged
merged 3 commits into from
Jul 9, 2021

Conversation

fabianfett
Copy link
Member

@fabianfett fabianfett commented Jul 7, 2021

Motivation

In our HTTPConnectionPool.StateMachine we want to manage HTTPConnection states. However, to ensure that our state machine reflects the outside world it is beneficial if the state machine would hold references to the actual existing connections. Exposing our Connection types directly to the state machine could lead to abuse though. Developers might call actions directly on the Connection within the state machine.

Modifications

  • Add a HTTPConnectionPool.Connection type that is a box around an actual connection. The purpose of the box is to ensure no actions are invoked on the connection within the state machine. Further the box can be used for testing the state machine without creating actual connections.

Caveats

  • This PR doesn't add any tests. This code will be thoroughly tested once the actual state machines land. (This code is not hit for actual uses as of today)

@fabianfett fabianfett added the 🔨 semver/patch No public API change. label Jul 7, 2021
@fabianfett fabianfett added this to the HTTP/2 support milestone Jul 7, 2021
enum HTTPConnectionPool {
struct Connection {
struct Connection: Equatable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be Hashable.

private enum Reference {
// case http1_1(HTTP1Connection)

#if DEBUG
Copy link
Collaborator

Choose a reason for hiding this comment

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

We sometimes want to run tests in release mode: we should probably just define this always and name it to indicate its use in testing.

}
}

#if DEBUG
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't hide this behind debug: use underscores and testOnly (e.g. __testOnly_channel.)

@fabianfett fabianfett force-pushed the ff-add-connection-box branch 2 times, most recently from f12c141 to 01cce0c Compare July 8, 2021 13:25
@fabianfett fabianfett requested a review from Lukasa July 8, 2021 13:53
@fabianfett fabianfett force-pushed the ff-add-connection-box branch 2 times, most recently from 67b4791 to 0856060 Compare July 8, 2021 15:40
@fabianfett fabianfett force-pushed the ff-add-connection-box branch from 41e4881 to 77fdb04 Compare July 8, 2021 16:58
@fabianfett fabianfett merged commit 152c21e into swift-server:main Jul 9, 2021
@fabianfett fabianfett deleted the ff-add-connection-box branch July 9, 2021 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants