Add additional NIOAsyncChannel config #406
Conversation
Motivation: Recent additions to pipeline configuration did not expose some of the `NIOAsyncChannel` configuration. Modifications: Expose `backpressureStrategy`, `isOutboundHalfClosureEnabled`. I think specific naming and object encapsulation for these parameters should be discussed later before SPI is removed and we review the API as a whole. Result: Pipelie helpers for `NIOAsyncChannel` expose `backpressureStrategy`, `isOutboundHalfClosureEnabled`.
rnro
force-pushed
the
async_channels_fixups
branch
from
0647d28
to
c46c98f
Compare
| backpressureStrategy: NIOAsyncSequenceProducerBackPressureStrategies.HighLowWatermark? = nil, | ||
| isOutboundHalfClosureEnabled: Bool = false, | ||
| inboundType: Inbound.Type, | ||
| outboundType: Outbound.Type, |
There was a problem hiding this comment.
can we reorder these so {in,out}boundType come first? Required parameters should come first.
There was a problem hiding this comment.
We should add defaults to the types as well like this = Inbound.self. Then the order is the same as in our bootstrap
There was a problem hiding this comment.
I don't think we want to add defaults here to force people to consider what types are used. This was also mentioned in an earlier PR #403 (comment)
There was a problem hiding this comment.
This is not really a default. If you write inboundType: Inbound.Type = Inbound.self it will allow the user to either write out the generic return type which will get inferred or pass parameters. If you don't add this default value then they always have to write out the parameters even though type inference doesn't need it.
There was a problem hiding this comment.
Yep, good point. I've added these in the latest commit and put the parameters back in the order to match NIOAsyncChannel.
| @@ -258,10 +258,10 @@ extension NIOHTTP2Handler { | |||
| @available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) | |||
| @_spi(AsyncChannel) | |||
| public func createStreamChannel<Inbound, Outbound>( | |||
| inboundType: Inbound.Type = Inbound.self, | |||
There was a problem hiding this comment.
This order seems different than the other methods
rnro
force-pushed
the
async_channels_fixups
branch
from
4e74dde
to
49eb2f9
Compare
|
The API breakages are safe because these changes were added since the last release. |
1 similar comment
|
The API breakages are safe because these changes were added since the last release. |
rnro
added
the
needs-no-version-bump
Motivation:
Recent additions to pipeline configuration did not expose some of the
NIOAsyncChannelconfiguration.Modifications:
Expose
backpressureStrategy,isOutboundHalfClosureEnabled.I think specific naming and object encapsulation for these parameters should be discussed later before SPI is removed and we review the API as a whole.
Result:
Pipelie helpers for
NIOAsyncChannelexposebackpressureStrategy,isOutboundHalfClosureEnabled.