Skip to content

Add a frame delegate #522

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

Merged
merged 5 commits into from
Jul 18, 2025
Merged

Add a frame delegate #522

merged 5 commits into from
Jul 18, 2025

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Jul 17, 2025

Motivation:

Frames written from a child channel may not be written immediately by the connection channel. For example, a DATA frame written on a stream may be larger than the max frame size imposed on the connection and so the connection channel will have to slice it into multiple frames. Or the connection may not be writable and may delay the transmission of the frame. This behaviour isn't currently observable but is useful to know about.

Modifications:

  • Add a frame delegate which is notified when certain frames are written by the connection channel.

Result:

Users can observer when headers and data frames are written into the connection channel.

Motivation:

Frames written from a child channel may not be written immediately by
the connection channel. For example, a DATA frame written on a stream
may be larger than the max frame size imposed on the connection and so
the connection channel will have to slice it into multiple frames. Or
the connection may not be writable and may delay the transmission of the
frame. This behaviour isn't currently observable but is useful to know
about.

Modifications:

- Add a frame delegate which is notified when certain frames are written
  by the connection channel.

Result:

Users can observer when headers and data frames are written into the
connection channel.
@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Jul 17, 2025
@@ -113,6 +113,9 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {
/// The maximum number of sequential CONTINUATION frames.
private let maximumSequentialContinuationFrames: Int

/// A delegate which is told about frames which have eebn written.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// A delegate which is told about frames which have eebn written.
/// A delegate which is told about frames which have been written.

case .byteBuffer(let buffer):
delegate.wroteData(buffer, endStream: data.endStream, streamID: frame.streamID)
case .fileRegion:
()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we take no action here? If it's intended should it be documented that we don't notify for file regions?

@@ -2844,6 +2848,72 @@ class SimpleClientServerFramePayloadStreamTests: XCTestCase {
XCTAssertNoThrow(try self.clientChannel.finish())
XCTAssertNoThrow(try self.serverChannel.finish())
}

func testFrameDelegateIsCalledForDATAFrames() throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to also assert on header frames, is this name appropriate?

/// This delegate, when used by the ``NIOHTTP2Handler`` will be called on the event
/// loop associated with the channel that the handler is a part of. As such you should
/// avoid doing expensive or blocking work in this delegate.
public protocol NIOHTTP2FrameDelegate: Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the requirement that this is Sendable coming from?

@glbrntt glbrntt requested review from Lukasa and rnro July 18, 2025 12:36
@Lukasa Lukasa merged commit 5e9e99e into apple:main Jul 18, 2025
40 checks passed
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.

3 participants