Skip to content

Add extension frame support. #476

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
Mar 8, 2017
Merged

Add extension frame support. #476

merged 5 commits into from
Mar 8, 2017

Conversation

Lukasa
Copy link
Member

@Lukasa Lukasa commented Mar 7, 2017

This PR adds support for emitting events when we receive extension frames.

Resolves #435.

The first few commits in here should be backported to older versions of hyper-h2 to allow them to use the 5.0 release series of hyperframe if needed.

@Lukasa
Copy link
Member Author

Lukasa commented Mar 7, 2017

Specifically, the first two commits are the ones that need backporting.

@Lukasa Lukasa force-pushed the add-extension-frame-support branch from 4571002 to dcfcf34 Compare March 7, 2017 14:10
Copy link
Contributor

@alexwlchan alexwlchan left a comment

Choose a reason for hiding this comment

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

Looks good on an initial skim, will review properly later.

Copy link
Contributor

@alexwlchan alexwlchan left a comment

Choose a reason for hiding this comment

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

One suggestion about logging consistency, otherwise LGTM.

h2/connection.py Outdated
"""
# All we do here is log.
self.config.logger.debug(
"Received unknown extension frame (ID 0x%x)", frame.stream_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Elsewhere we log the stream ID as a %d integer. Should we be consistent in the way we log?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm, probably, yeah.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I would probably lean towards doing logging in hex as you did it originally, but let’s make that a separate patch.

@Lukasa Lukasa force-pushed the add-extension-frame-support branch from dcfcf34 to 04cf12d Compare March 8, 2017 09:27
@alexwlchan
Copy link
Contributor

Okay, LGTM.

@Lukasa I’ll let you merge, so you can sort out backports and the like.

@Lukasa
Copy link
Member Author

Lukasa commented Mar 8, 2017

Cool, thanks @alexwlchan! I'm going to try to write this in as a shell script that we can commit to the repo so that backporting gets a bit easier.

@Lukasa Lukasa merged commit beef404 into master Mar 8, 2017
@Lukasa Lukasa deleted the add-extension-frame-support branch March 8, 2017 09:53
Lukasa added a commit that referenced this pull request Mar 8, 2017
@Lukasa Lukasa mentioned this pull request Mar 8, 2017
Lukasa added a commit that referenced this pull request Mar 8, 2017
@Lukasa Lukasa mentioned this pull request Mar 8, 2017
Lukasa added a commit that referenced this pull request Mar 8, 2017
@Lukasa Lukasa mentioned this pull request Mar 8, 2017
Lukasa added a commit that referenced this pull request Mar 8, 2017
Lukasa added a commit that referenced this pull request Mar 8, 2017
Lukasa added a commit that referenced this pull request Mar 8, 2017
Lukasa added a commit that referenced this pull request Mar 8, 2017
Lukasa added a commit that referenced this pull request Mar 9, 2017
Lukasa added a commit that referenced this pull request Mar 9, 2017
Lukasa added a commit that referenced this pull request Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants