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.

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

Draft 14 framer stream #21

Merged
merged 6 commits into from Oct 27, 2014
Merged

Conversation

mad-p
Copy link
Collaborator

@mad-p mad-p commented Oct 23, 2014

Updates for framer, stream, and connection.

Unfortunately, tests fail between these two commits.
I recommend squash-before-merge (so that git bisect works).

If you find commits are too large, please tell me so. I'll work harder to make commits smaller.

Kaoru Maeda added 2 commits October 23, 2014 19:53
- 24-bit frame size
- Default max frame size is 16384, not 16383
- Setting identifier is now 16-bit
- Remove PAD_HIGH, padding in CONTINUATION
- Allow extension frames, extension settings
- Priority parameters are weight, stream_dependency, exclusive
- Stream statemachine adjusted for h2-14
- PRIORITY after stream close allowed
- Header block continuation handling
- Splitting large frame into smaller (DATA, HEADERS, PUSH_PROMISE)
- Pseudo headers at the beginning
def encode(frame)
frames = [frame]
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be a conditional? I.e. if you pass an array then it shouldn't be wrapped in another array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method expects only one frame as input.
When encoding HEAD or PUSH_PROMISE, there might be CONTINUATIONs when the compressed header block is too large.

Copy link
Owner

Choose a reason for hiding this comment

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

I think I'm still missing something.. If we expect to process only one frame, then why add the logic to iterate or an array? On the other hand, if you pass in an array, 338 will end up wrapping an array inside of another array.. that you'd have to flatten?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found what's wrong with me. The lines should read

if frame is HEADERS or PUSH_PROMISE
  frames = encode_headers(frame) # this creates more than one frame
else
  frames = [frame]
end
frames.map { loop over frames }

L.338 should not pretend to be a common case. It should be in an else clause.
Does this explain well?

@igrigorik
Copy link
Owner

Overall, lgtm! Just a couple of small nits and questions...

P.S. Kudos for the great work!

@mad-p
Copy link
Collaborator Author

mad-p commented Oct 26, 2014

Travis build failed with an syntax error. I'll work on this.

0bd0fae fixes this

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling abf38f8 on lepidum:draft-14-framer-stream into da622d2 on igrigorik:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a93f2fd on lepidum:draft-14-framer-stream into da622d2 on igrigorik:master.

igrigorik added a commit that referenced this pull request Oct 27, 2014
@igrigorik igrigorik merged commit edabe01 into igrigorik:master Oct 27, 2014
@igrigorik
Copy link
Owner

great stuff, thanks!

@mad-p mad-p deleted the draft-14-framer-stream branch October 29, 2014 05:51
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.

None yet

3 participants