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

Multiple edits (per the interim) #132

Closed
wants to merge 22 commits into from
Closed

Conversation

jasnell
Copy link
Contributor

@jasnell jasnell commented Jun 17, 2013

There are quite a few edits in here... merges from the other outstanding pull requests included... It was easier just to merge these and work on them iteratively given the extensive changes...

jasnell and others added 12 commits June 14, 2013 11:34
Specifically calls them out as connection errors of type
PROTOCOL_ERROR.
Per the interim.. dealing with frame size limits
Move definition of FINAL flag to DATA and HEADERS frames.
Replace HEADERS+PRIORITY with HEADERS and a separate PRIORITY flag.
Renumber other associated flags now that FINAL is frame-specific.
Add a MSG_DONE flag for future extensibility.
or if the size exceeds a strictly defined limit, the endpoint MUST
send a FRAME_TOO_LARGE error. If the frame stream identifier is 0x0,
this code MUST be sent as a connection error. If the stream
identifier is anything other than 0x0, it is sent as stream error.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is going to be so verbose, then you need to include the fact that HEADERS and PUSH_PROMISE also require that the connection be broken.

@martinthomson
Copy link
Collaborator

Can you try this one again? I think that you didn't properly rebase your checked out copy before making changes. This would be a hard commit to track.

@jasnell
Copy link
Contributor Author

jasnell commented Jun 17, 2013

@jpinner Yes, I know, we need to fully address the On or About issue before that would make much sense tho. This is an incremental change... We still need to figure out the more extensive edits.

@martinthomson I'm trying to figure out the best way to handle these edits. They are very extensive at this point, and it's only going to get more complex before we're done, especially with multiple people submitting overlapping pull requests...

Clear(er) explanation about associated streams with PUSH_PROMISE..
clear examples showing the use of RST_STREAM and PRIORITY with and
without ASSOCIATED_ONLY set.
@martinthomson
Copy link
Collaborator

I definitely want to see a separate commit for the ASSOCIATED flags on PRIORITY and RST_STREAM.

There are other items here as well that make it hard to review this change set.

Assign the default priority 2^30-1 as agreed to at the interim
@jasnell
Copy link
Contributor Author

jasnell commented Jun 17, 2013

Yes, these changes are going to be quite extensive. Given the time constraints, I'm going to keep working in my branch on these edits, tho. Once they're done, I'll come up with a better approach to attempt an incremental review.

@jasnell jasnell closed this Jun 17, 2013
@willchan
Copy link
Contributor

"Per the interim... address frame size limits. 64k in the general case, 16k in HTTP. individual frame types and usage can impose additional limits."

Please update to explain this in detail. I know we agreed on this in the interim meeting, but I'd like the full explanation documented in writing for posterity.

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

6 participants