igrigorik / spdy

SPDY is a protocol designed to reduce latency of web pages
315 stars 23 forks source link

Frame building API #13

Closed jonasschneider closed 12 years ago

jonasschneider commented 12 years ago

What came to my mind after thinking about the 'return parsed packets' pull:

It's a good idea to abstract the BinData records for parsing packets. Maybe there should also be some kind of 'Framing API', to also abstract the internals for when creating frames... what do you think?

igrigorik commented 12 years ago

I recall having the same idea when I was fiddling with implementing a spdy server way back.. Do you have any ideas for the API? If we can improve the interface.. could be interesting.

jonasschneider commented 12 years ago

Maybe the simplest way would be to also hide the BinData structs, and return binary strings directly:

f = SPDY::Framer.new(@zlib)
f.syn_stream(stream: 1, associated_stream: 3) #=> "\x80\x02..."
f.headers(stream: 1, headers: {'a' => 'b'}) #=> "\x80\x02..."
f.data_frame(stream: 1, data: 'Hi there') #=> "\x00\x00..."

Then we are flexible with the implementation, and open to maybe rewriting it as a C extension someday, or whatever.

igrigorik commented 12 years ago

I like the idea of abstracting away the BinData guts.. since as you pointed out, that decouples us down the road from swapping in a different implementation.

(I have nothing against bindata, but I chose it originally due to its simplicity to prototype with, over anything else)

Having said that, having an API which spits out the binary strings directly is also a bit inflexible - no? Just for sake of evaluating the alternatives:

SPDY::Protocol::Data::Frame.create( { ... } ) - we could keep the objects but create a single step function to emit the binary string. It's a bit more verbose with all the namespaces, but the upside is that it's very explicit.

For the frame api, if we were to go with that approach, I'd keep the .method on the framer as exact packet names, to remove any ambiguity: .synstream, .headers, .data, etc.