Skip to content

Fix OOM issue when setting concurrentHTTP1ConnectionsPerHostSoftLimit to Int.max#763

Merged
fabianfett merged 3 commits into
swift-server:mainfrom
aryan-25:fix-connection-per-host-limit-OOM
Aug 14, 2024
Merged

Fix OOM issue when setting concurrentHTTP1ConnectionsPerHostSoftLimit to Int.max#763
fabianfett merged 3 commits into
swift-server:mainfrom
aryan-25:fix-connection-per-host-limit-OOM

Conversation

@aryan-25

@aryan-25 aryan-25 commented Aug 8, 2024

Copy link
Copy Markdown
Contributor

Motivation:

When a user wishes to make the connection pool create as many concurrent connections as possible, a natural way to achieve this would be to set .max to the concurrentHTTP1ConnectionsPerHostSoftLimit property.

HTTPClient.Configuration().connectionPool = .init(
    idleTimeout: .hours(1),
    concurrentHTTP1ConnectionsPerHostSoftLimit: .max
)

The concurrentHTTP1ConnectionsPerHostSoftLimit property is of type Int. Setting it to Int.max leads to Int.max being passed as an argument to Arrays .reserveCapacity(_:) method, causing an OOM issue.

Addresses Github Issue #751

Modifications:

Capped the argument to self.connections.reserveCapacity(_:) to 1024 in HTTPConnectionPool.HTTP1Connections

Result:

Users can now set the concurrentHTTP1ConnectionsPerHostSoftLimit property to .max without causing an OOM issue.

…t` to `Int.max` in HTTPClient.Configuration.ConnectionPool
XCTAssertNoThrow(try httpClient.syncShutdown())
}

let response = httpClient.get(url: "https://localhost:\(bin.port)", deadline: .now() + .seconds(2))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we need to use the async/await API here.

@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Aug 13, 2024

@Lukasa Lukasa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Love this, very nice.

@fabianfett
fabianfett merged commit 4b7a68e into swift-server:main Aug 14, 2024
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.

3 participants