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.

Sign up for GitHub

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

Jump to bottom

Only flush when necessary in the HTTP2ChannelHandler #320

Merged
merged 1 commit into from
Dec 15, 2021
Merged

Only flush when necessary in the HTTP2ChannelHandler #320

merged 1 commit into from
Dec 15, 2021

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Dec 14, 2021

Motivation:

The wroteFrame flag in HTTP2ChannelHandler indicates whether a frame
has been written. This flag is checked before flushing in order to avoid
unnecessary flushes. Unfortunately this is only ever set to true.

Modifications:

  • Add a flushIfNecessary to check and toggle wroteFrame and flush if
    necessary
  • Add a test

Result:

@glbrntt
Only flush when necessary in the HTTP2ChannelHandler
e79e04d 
Motivation:

The `wroteFrame` flag in `HTTP2ChannelHandler` indicates whether a frame
has been written. This flag is checked before flushing in order to avoid
unnecessary flushes. Unfortunately this is only ever set to `true`.

Modifications:

- Add a `flushIfNecessary` to check and toggle `wroteFrame` and flush if
  necessary
- Add a test

Result:

- Few unnecessary flushes
- Resolves apple#245
@glbrntt glbrntt requested a review from Lukasa December 14, 2021 14:14
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Dec 14, 2021
@glbrntt glbrntt requested a review from FranzBusch December 14, 2021 14:14
FranzBusch
FranzBusch approved these changes Dec 14, 2021
View reviewed changes
Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Lukasa
Lukasa approved these changes Dec 14, 2021
View reviewed changes
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.

Really nice fix, thanks @glbrntt.

@Lukasa
Copy link
Contributor

Lukasa commented Dec 14, 2021

@swift-nio-bot test this please

@glbrntt
Copy link
Contributor Author

glbrntt commented Dec 15, 2021

Nightly is failing an allocation counting test. We get the same failure on nightly without this change (see #321).

Going to merge over the failure and look into the regression separately.

@glbrntt glbrntt merged commit 39a0aa4 into apple:main Dec 15, 2021
@glbrntt glbrntt deleted the gb-wrote-frame branch December 15, 2021 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lukasa Lukasa Lukasa approved these changes

@FranzBusch FranzBusch FranzBusch approved these changes

Assignees
No one assigned
Labels
🔨 semver/patch No public API change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HTTP2ChannelHandler.wroteFrame is never returned to false
3 participants
@glbrntt @Lukasa @FranzBusch