Add new protocol for ChannelHandler to get buffered bytes in the channel handler #2918
Conversation
…ohnnzhou/swift-nio into channel_handler_buffered_bytes
|
Thanks so much for opening this @johnnzhou! cc @weissi @Joannis @FranzBusch who are all going to care about this API. |
Sources/NIOCore/ChannelHandler.swift
Outdated
| public protocol OutboundBufferedBytesAuditableChannelHandler { | ||
| /// Returns the number of bytes buffered in the channel handler, which are queued to be sent to | ||
| /// the next outbound channel handler. | ||
| func auditOutboundBufferedBytes() -> Int |
There was a problem hiding this comment.
Minor nit: these should probably be computed properties, not functions.
There was a problem hiding this comment.
Only if we want to add an O(1) requirement. I don't we think we want though because some ChannelHandlers have n child ChannelHandlers which means they wouldn't be able to conform, strictly speaking.
It would also be unnecessarily different from the other overloads that take an argument.
Sources/NIOCore/ChannelHandler.swift
Outdated
|
|
||
| /// A `OutboundBufferedBytesAuditableChannelHandler` is a `ChannelHandler` that | ||
| /// audits and reports the number of bytes buffered for outbound direction. | ||
| public protocol OutboundBufferedBytesAuditableChannelHandler { |
There was a problem hiding this comment.
Per our API evolution rules, these will need to be prefixed with NIO:
| public protocol OutboundBufferedBytesAuditableChannelHandler { | |
| public protocol NIOOutboundBufferedBytesAuditableChannelHandler { |
Sources/NIOCore/ChannelHandler.swift
Outdated
|
|
||
| /// A `InboundBufferedBytesAuditableChannelHandler` is a `ChannelHandler` that | ||
| /// audits and reports the number of bytes buffered for inbound direction. | ||
| public protocol InboundBufferedBytesAuditableChannelHandler { |
There was a problem hiding this comment.
| public protocol InboundBufferedBytesAuditableChannelHandler { | |
| public protocol NIOInboundBufferedBytesAuditableChannelHandler { |
| @@ -1492,6 +1492,8 @@ public enum ChannelPipelineError: Error { | |||
| case alreadyRemoved | |||
| /// `ChannelHandler` was not found. | |||
| case notFound | |||
| /// `ChannelHandler` is not auditable. | |||
| case notAuditable | |||
There was a problem hiding this comment.
We're not going to be able to add a new error case here: we'll either need a new error, or we'll need to find a way to use one we already have.
| @@ -2089,3 +2091,171 @@ extension ChannelPipeline: CustomDebugStringConvertible { | |||
| return handlers | |||
| } | |||
| } | |||
|
|
|||
| extension ChannelPipeline { | |||
There was a problem hiding this comment.
These APIs will definitely want syncOperations versions.
| /// | ||
| /// - parameters: | ||
| /// - name: the name of the channel handler whose outbound buffered bytes will be audited. | ||
| public func auditOutboundBufferedBytes(name: String) -> EventLoopFuture<Int> { |
There was a problem hiding this comment.
I'm not sure this API is pulling its weight: users can always obtain channel handlers by name and call the relevant functions directly.
| /// | ||
| /// - parameters: | ||
| /// - handler: the channel handler object whose outbound buffered bytes will be audited. | ||
| public func auditOutboundBufferedBytes(handler: ChannelHandler) -> EventLoopFuture<Int> { |
There was a problem hiding this comment.
Same here: this API is just a function call on the handler directly, I don't think any extra API surface is needed for this.
| } | ||
| } | ||
|
|
||
| private func audit0(context: ChannelHandlerContext, direction: AuditDirection) -> Result<Int, Error> { |
There was a problem hiding this comment.
I'm not sure this needs an error result for not being able to do the type conversion, it might just want to return Int?
There was a problem hiding this comment.
It could just throw if it really wants to propagate an error.
There was a problem hiding this comment.
Given that it is not a good design choice to add an error in ChannelPipelineError and error conveys the same outcome as Int?, I agree it is better to return Int? directly.
Hi @FranzBusch, thanks for this suggestion. I originally designed this functionality as a way for users to inspect the number of bytes buffered in the channel handlers. Perhaps How about we use the word What do you think? |
|
Would this PR also (intend to) support child channels, such as in HTTP/2. |
Do we need count even here? What about
Depends what you expect. Every |
I'm generally ok with
My original plan is to implement this protocol for channel handlers provided by NIO. But maybe we need to make separate PRs for each handler? |
I understand where you are coming from. I personally still prefer that name but I would like to hear what @Lukasa @glbrntt or @weissi think.
We should definitely separate the API additions in this PR from the adoption. |
|
I'm happy for us to go with |
|
Sounds good. I will make the change accordingly. I will add more test cases in other commits. |
Add new protocol to get buffered bytes from
ChannelHandlers andChannelPipelineAPI to query the buffered bytes fromChannelHandlersMotivation:
In #2849, a new
ChannelOptionis introduced to retrieve the number of buffered outbound bytes in aChannel. However, this solution does not account for bytes that may be buffered within individualChannelHandlers. This PR builds on #2849 by adding functionality to audit buffered bytes residing inChannelHandlers and exposing this information through newChannelPipelineAPIs.Modifications:
ChannelHandlerto audit buffered bytes for inbound and outbound.NIOOutboundBufferedBytesAuditableChannelHandlerNIOInboundBufferedBytesAuditableChannelHandlerChannelPipelineAPIsResult:
Users can now easily query the amount of bytes buffered in
ChannelHandlers using the newChannelPipelineAPIs, enhancing the visibility ofChannelHandlerperformance.I'd like to make PR to gather some feedback from the community regarding the design before proceeding with further implementation. I'm planning to add more test cases and make existing
ChannelHandlers in SwiftNIO project conform to new protocols.