Enable strict concurrency checking for NIOHTTP1 #3115
Conversation
rnro
force-pushed
the
strict_concurrency_niohttp1
branch
from
9f4bfc7 to
71328d5
Compare
Motivation: To ensure NIOHTTP1 concurrency safety. Modifications: * Enable strict concurrency checking in the package manifest. * Mark several objects `Sendable` with `@preconcurrency` annotations where they are returned in futures which may execute in arbitrary concurrency domains. * `HTTPServerProtocolUpgrader` * `NIOHTTPClientProtocolUpgrader` * `NIOTypedHTTPClientProtocolUpgrader` * `NIOUpgradableHTTPServerPipelineConfiguration` * `NIOUpgradableHTTPClientPipelineConfiguration` * `NIOTypedHTTPClientUpgradeConfiguration` * Mark handlers as explicitly not sendable * `NIOTypedHTTPClientUpgradeHandler` * `NIOTypedHTTPServerUpgradeHandler` Result: No more concurrency warnings. Builds will warn and CI will fail if regressions are introduced.
* Make use of `assumeIsolated` instead of `NIOLoopBound`s * Do not add `Sendable` requirement to `NIOHTTPClientProtocolUpgrader` * Do not add `Sendable` requirement to `HTTPServerProtocolUpgrader`
rnro
force-pushed
the
strict_concurrency_niohttp1
branch
from
58292a9 to
073e64e
Compare
| return { | ||
| self.upgradeState = .upgrading | ||
|
|
||
| // assumeIsolated is safe here because this is only called from channelRead |
There was a problem hiding this comment.
Isn't the justification here that removeHTTPHandlers returns us a future on the right EL? Should it be modified to return an isolated ELF? Otherwise the guarantee is based on documentation vs types.
| upgrader: any NIOTypedHTTPServerProtocolUpgrader<UpgradeResult>, responseHeaders: HTTPHeaders, | ||
| proto: String |
There was a problem hiding this comment.
Given the weird formatting can we do one param per line?
| upgrader: any NIOTypedHTTPServerProtocolUpgrader<UpgradeResult>, responseHeaders: HTTPHeaders, | |
| proto: String | |
| upgrader: any NIOTypedHTTPServerProtocolUpgrader<UpgradeResult>, | |
| responseHeaders: HTTPHeaders, | |
| proto: String |
There was a problem hiding this comment.
This one got a thumbs up but no action 🤷♂️
There was a problem hiding this comment.
Sorry, there must have been more than one instance or something - I definitely tweaked formatting in code that looked like this 4a5073e#diff-df1f0e00e4593ea99b5fed7e3a37f6415e9531d25dcb6994d6ba0f0a8ecf6cebR316
| /// - Returns: An `EventLoopFuture` that will contain a callback to invoke if upgrade is requested, or nil if upgrade has failed. Never returns a failed future. | ||
| /// - Returns: An isolated `EventLoopFuture` that will contain a callback to invoke if upgrade is requested, | ||
| /// or nil if upgrade has failed. Never returns a failed future. | ||
| /// The `NIOLoopBound` is safe because it's only called after the hop in firstRequestHeadReceived |
There was a problem hiding this comment.
This is left over from a previous implementation- removed.
| @@ -18,7 +18,8 @@ import NIOCore | |||
|
|
|||
| /// Configuration for an upgradable HTTP pipeline. | |||
| @available(macOS 13, iOS 16, tvOS 16, watchOS 9, *) | |||
| public struct NIOUpgradableHTTPServerPipelineConfiguration<UpgradeResult: Sendable> { | |||
| @preconcurrency | |||
There was a problem hiding this comment.
No need for a preconcurrency annotation here.
| @@ -146,7 +147,8 @@ extension ChannelPipeline.SynchronousOperations { | |||
|
|
|||
| /// Configuration for an upgradable HTTP pipeline. | |||
| @available(macOS 13, iOS 16, tvOS 16, watchOS 9, *) | |||
| public struct NIOUpgradableHTTPClientPipelineConfiguration<UpgradeResult: Sendable> { | |||
| @preconcurrency | |||
There was a problem hiding this comment.
No need for a preconcurrency annotation here.
| @@ -42,7 +43,8 @@ public protocol NIOTypedHTTPClientProtocolUpgrader<UpgradeResult> { | |||
|
|
|||
| /// The upgrade configuration for the ``NIOTypedHTTPClientUpgradeHandler``. | |||
| @available(macOS 13, iOS 16, tvOS 16, watchOS 9, *) | |||
| public struct NIOTypedHTTPClientUpgradeConfiguration<UpgradeResult: Sendable> { | |||
| @preconcurrency | |||
There was a problem hiding this comment.
No need for a preconcurrency annotation here.
| @@ -47,7 +48,8 @@ public protocol NIOTypedHTTPServerProtocolUpgrader<UpgradeResult> { | |||
|
|
|||
| /// The upgrade configuration for the ``NIOTypedHTTPServerUpgradeHandler``. | |||
| @available(macOS 13, iOS 16, tvOS 16, watchOS 9, *) | |||
| public struct NIOTypedHTTPServerUpgradeConfiguration<UpgradeResult: Sendable> { | |||
| @preconcurrency | |||
There was a problem hiding this comment.
No need for preconcurrency annotation here.
| }.flatMap { () -> EventLoopFuture<UpgradeResult> in | ||
| upgrader.upgrade(channel: channel, upgradeRequest: requestHead) | ||
| }.nonisolated().hop(to: context.eventLoop) | ||
| .assumeIsolated() |
There was a problem hiding this comment.
nonisolated().hop(to:).assumeIsolated() shouldn't do anything meaningful here. We were already isolated.
Motivation:
To ensure NIOHTTP1 concurrency safety.
Modifications:
Sendablewith@preconcurrencyannotations where they are returned in futures which may execute in arbitrary concurrency domains.NIOTypedHTTPClientProtocolUpgraderNIOTypedHTTPClientUpgradeConfigurationNIOUpgradableHTTPServerPipelineConfigurationNIOUpgradableHTTPClientPipelineConfigurationNIOTypedHTTPServerProtocolUpgraderNIOTypedHTTPServerUpgradeConfigurationNIOTypedHTTPClientUpgradeHandlerNIOTypedHTTPServerUpgradeHandlerNIOHTTPClientUpgradeSendableConfigurationNIOHTTPServerUpgradeSendableConfigurationResult:
No more concurrency warnings. Builds will warn and CI will fail if regressions are introduced.