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

Add sync option support to HTTP2StreamChannel #282

Merged
merged 1 commit into from
Mar 16, 2021

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Mar 15, 2021

Motivation:

SwiftNIO 2.27.0 added support for synchronous channel options allowing
callers to get options synchronously -- saving a future allocation -- if
they know they're on the correct event loop. 'HTTP2StreamChannel' should
support this.

Modifications:

  • Add 'HTTP2StreamChannel.SynchronousOptions' with conformance to
    'NIOSynchronousChannelOptions'

Result:

Callers can get and set options synchronously on 'HTTP2StreamChannel'

@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Mar 15, 2021
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

LGTM! Want to follow-up with a separate patch to use the sync options API on the underlying channel to get at the autoRead channel option when it's available?

}
}

private func setOption0<Option: ChannelOption>(_ option: Option, value: Option.Value) throws {
assert(eventLoop.inEventLoop)
self.eventLoop.assertInEventLoop()
Copy link
Contributor

Choose a reason for hiding this comment

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

As these are now called publicly through APIs where the user is required to get them correct, I think we should lift these up to preconditions.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Sorry, per earlier note I do have feedback here.

@glbrntt glbrntt force-pushed the gb-add-sync-options branch from 7cfb8cf to 320e4a3 Compare March 15, 2021 11:30
@glbrntt
Copy link
Contributor Author

glbrntt commented Mar 15, 2021

All the CI -- except for the nightly job, which is expected to fail -- run without failures until they hit an issue reporting the status.

@glbrntt glbrntt requested a review from Lukasa March 15, 2021 13:59
Sources/NIOHTTP2/HTTP2StreamChannel.swift Outdated Show resolved Hide resolved
Sources/NIOHTTP2/HTTP2StreamChannel.swift Outdated Show resolved Hide resolved
Sources/NIOHTTP2/HTTP2StreamChannel.swift Outdated Show resolved Hide resolved
Motivation:

SwiftNIO 2.27.0 added support for synchronous channel options allowing
callers to get options synchronously -- saving a future allocation -- if
they know they're on the correct event loop. 'HTTP2StreamChannel' should
support this.

Modifications:

- Add 'HTTP2StreamChannel.SynchronousOptions' with conformance to
  'NIOSynchronousChannelOptions'

Result:

Callers can get and set options synchronously on 'HTTP2StreamChannel'
@glbrntt glbrntt force-pushed the gb-add-sync-options branch from 320e4a3 to 7c2c119 Compare March 15, 2021 15:08
@glbrntt glbrntt requested a review from Lukasa March 15, 2021 15:10
@Lukasa
Copy link
Contributor

Lukasa commented Mar 16, 2021

@swift-nio-bot test this please

@Lukasa
Copy link
Contributor

Lukasa commented Mar 16, 2021

Merging over the nightly failure.

@Lukasa Lukasa merged commit 4e96d02 into apple:main Mar 16, 2021
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.

2 participants