swift-nio icon indicating copy to clipboard operation
swift-nio copied to clipboard

Add new protocol for ChannelHandler to get buffered bytes in the channel handler

Open johnnzhou opened this issue 1 year ago • 8 comments

Add new protocol to get buffered bytes from ChannelHandlers and ChannelPipeline API to query the buffered bytes from ChannelHandlers

Motivation:

In #2849, a new ChannelOption is introduced to retrieve the number of buffered outbound bytes in a Channel. However, this solution does not account for bytes that may be buffered within individual ChannelHandlers. This PR builds on #2849 by adding functionality to audit buffered bytes residing in ChannelHandlers and exposing this information through new ChannelPipeline APIs.

Modifications:

  • Two new protocols for ChannelHandler to audit buffered bytes for inbound and outbound.
    • NIOOutboundByteBufferingChannelHandler
    • NIOInboundByteBufferingChannelHandler
  • New ChannelPipeline APIs
    • outboundBufferedBytes()
    • outboundBufferedBytes(in: ChannelHandlerContext)
    • inboundBufferedBytes()
    • inboundBufferedBytes(in: ChannelHandlerContext)

Result:

Users can now easily query the amount of bytes buffered in ChannelHandlers using the new ChannelPipeline APIs, enhancing the visibility of ChannelHandler performance.

johnnzhou avatar Oct 13 '24 06:10 johnnzhou

Thanks so much for opening this @johnnzhou! cc @weissi @joannis @FranzBusch who are all going to care about this API.

Lukasa avatar Oct 14 '24 13:10 Lukasa

I am a bit unsure if the word audit is the right one here. Could we just drop that from all the APIs here? What do others think?

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 audit is more suited for financial contexts.

How about we use the word count instead? For instance, we could rename the protocol to NIOOutboundBufferedBytesCountableChannelHandler and the public method to countOutboundBufferedBytes. This change would preserve the idea of inspection while emphasizing the action of counting, making it more intuitive.

What do you think?

johnnzhou avatar Oct 16 '24 06:10 johnnzhou

Would this PR also (intend to) support child channels, such as in HTTP/2.

Joannis avatar Oct 16 '24 07:10 Joannis

How about we use the word count instead? For instance, we could rename the protocol to NIOOutboundBufferedBytesCountableChannelHandler and the public method to countOutboundBufferedBytes. This change would preserve the idea of inspection while emphasizing the action of counting, making it more intuitive.

Do we need count even here? What about NIOOutboundByteBufferingChannelHandler? The current name of the property sounds good to me.

Would this PR also (intend to) support child channels, such as in HTTP/2.

Depends what you expect. Every ChannelHandler that does multiplexing such as the H2Handler needs to decide how to treat. When a handler in a stream channel asks about the outbound buffered bytes it could return both the buffer from just the stream or the whole connection. We need to decide and implement that on a case-by-case basis.

FranzBusch avatar Oct 16 '24 08:10 FranzBusch

Do we need count even here? What about NIOOutboundByteBufferingChannelHandler? The current name of the property sounds good to me.

I'm generally ok with NIOOutboundByteBufferingChannelHandler. However, my main concern is that this name might be a bit misleading. Two new protocols are not intended to help provide buffering mechanism or manage bytes storage in the channel handlers. Instead, their main function is to collect the number of bytes buffered in the channel handler and allow users to make informed decision on their network load.

Would this PR also (intend to) support child channels, such as in HTTP/2.

Depends what you expect. Every ChannelHandler that does multiplexing such as the H2Handler needs to decide how to treat. When a handler in a stream channel asks about the outbound buffered bytes it could return both the buffer from just the stream or the whole connection. We need to decide and implement that on a case-by-case basis.

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?

johnnzhou avatar Oct 17 '24 06:10 johnnzhou

I'm generally ok with NIOOutboundByteBufferingChannelHandler. However, my main concern is that this name might be a bit misleading. Two new protocols are not intended to help provide buffering mechanism or manage bytes storage in the channel handlers. Instead, their main function is to collect the number of bytes buffered in the channel handler and allow users to make informed decision on their network load.

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.

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?

We should definitely separate the API additions in this PR from the adoption.

FranzBusch avatar Oct 17 '24 08:10 FranzBusch

I'm happy for us to go with NIOOutboundByteBufferingChannelHandler

Lukasa avatar Oct 17 '24 12:10 Lukasa

Sounds good. I will make the change accordingly. I will add more test cases in other commits.

johnnzhou avatar Oct 17 '24 17:10 johnnzhou