HEADERS parser #12

Merged
merged 6 commits into from Jan 4, 2012

Projects

None yet

2 participants

@jonasschneider
Collaborator

This adds support for the HEADERS packet.

@igrigorik
Owner

Hmm, this is a bit confusing.. We have the headers coming in SYN_STREAM, which fire on_headers_complete, but then we have a separate on_headers, which is invoked for the HEADERS packets... If memory serves, draft 3 is suppose to reconcile this case by separating the headers out of SYN_STREAM into its own dedicated packet?

For sanity, I'd love to keep a consistent interface (although based on draft 3, this may be a non-issue). For example, since we're basically saying the stream can be augmented with any number of headers at any point (application specific semantics), then perhaps we shouldn't even have the on_headers_complete, since that's just misleading..

@jonasschneider
Collaborator

Yep, the notes on http://www.chromium.org/spdy/spdy-protocol contain this as an expected change for draft 3.

Move the Name/Value blocks out of SYN_STREAM and SYN_REPLY; instead they only exist in HEADERS.

I personally am not sure if this is the right way to go; if a large body is being POSTed, the request can only be processed after the whole body has been sent, as additional HEADERS frames are allowed to come in at any time. But hey, it's the spec.

How about we remove on_headers_complete, but instead add on_stream_establish (or something like that) and
on_headers? When a draft 2 SYN_STREAM arrives, we call on_stream_establish and immediately afterwards on_headers with the headers contained in the packet. Also, we call on_headers when a HEADERS frame arrives.

For draft 3, we can then just stop firing the on_headers callback for SYN_STREAMs.

@igrigorik
Owner

The fact that there are two distinct ways to send headers, to me, is a sign that it should be refactored, so I'm all for the draft 3 change. In terms of changes, let's do the following:

  • Remove on_headers_complete
  • Add on_headers and fire the callback on SYN_STREAM and HEADERS packets, this way, once draft 3 comes around we'll just remove the SYN_STREAM and preserve the api
  • How about on_open for new connections, to reuse the language from websocket's, etc.
@jonasschneider
Collaborator

That's about what I wanted to say.

@jonasschneider
Collaborator

Once we go draft 3, we can then just remove unpack_control(pckt, data) and move the call to pckt.parse into try_parse.

@igrigorik igrigorik commented on an outdated diff Jan 4, 2012
lib/spdy/parser.rb
@@ -38,12 +42,10 @@ def on_reset(&blk)
def unpack_control(pckt, data)
pckt.parse(data)
-
- if @on_headers_complete
- @on_headers_complete.call(pckt.header.stream_id.to_i,
- (pckt.associated_to_stream_id.to_i rescue nil),
- (pckt.pri.to_i rescue nil),
- pckt.uncompressed_data.to_h)
+
+ if @on_headers && pckt.uncompressed_data.to_h != {}
@igrigorik
igrigorik Jan 4, 2012 Owner

pedantic, but let's call to_h line higher to avoid creating two hashes (one in if, one lower), and to_h.empty?

@jonasschneider
Collaborator

Done.

@igrigorik igrigorik merged commit 5b74597 into igrigorik:master Jan 4, 2012
@igrigorik
Owner

Great stuff, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment