igrigorik / http-2

Pure Ruby implementation of HTTP/2 protocol
https://httpwg.github.io/specs/rfc7540.html
MIT License
894 stars 63 forks source link

Could we add flags to headers/data event? #149

Closed xiejiangzhi closed 2 years ago

xiejiangzhi commented 5 years ago

When I forward a gRPC request, I must send 3 frames(headers -> data -> headers). I think add flags of frame to event of data/headers is a easy way.

How do you think so? If you think it is ok, will create a PR for it.

igrigorik commented 5 years ago

When I forward a gRPC request, I must send 3 frames(headers -> data -> headers).

As in, you need to send a trailer? You're not allowed to interleave headers and data frames within a stream.

I think add flags of frame to event of data/headers is a easy way.

How or why would this address what you're after?

xiejiangzhi commented 5 years ago

@igrigorik According to the GRPC spec https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses

HEADERS (flags = END_HEADERS)
:status = 200
grpc-encoding = gzip
content-type = application/grpc+proto

DATA
<Length-Prefixed Message>

HEADERS (flags = END_STREAM, END_HEADERS)
grpc-status = 0 # OK
trace-proto-bin = jher831yy13JHy3hc

Because I need know headers & data of request, check all frame will take more time.

remote_server_stream.on(:headers) do |headers|
  user_stream.headers(headers, end_stream: stream_is_end, end_header: headers_is_end?)
end

remote_server_stream.on(:data) do |data|
  user_stream.data(data, end_stream: stream_is_end?)
end

Other way is collecting all headers & data to an array, and send them when remote_server_stream close. Then, I cannot forward them real-time.

Because I didn't take more time to read code & understand http2, I didn't process it by frame_received event.

So which way do you think is better?

igrigorik commented 5 years ago

You definitely shouldn't buffer. What you're doing by listening to on(:headers) is correct: that will fire when you first receive the headers, and it will fire again when the trailer is received. For the latter case, you can check stream status stream.closed? or stream.status from inside of the headers callback. I don't think you need to get the flags. Does that make sense?

xiejiangzhi commented 5 years ago

Yes, it's right if the just for gRPC. But I want to forward all h2 request. I think stream.close cannot help me to do it when a normal stream that only send one header. And I ave checked the stream.closed?, it always be false in on(:headers) and on(:data)

conn.on(:frame_received) { |f| puts " <- #{f.inspect}" }
stream.on(:headers) do |h|
  headers.push(*h)
  puts " -- stream.closed? #{stream.closed?}"
end

stream.on(:data) do |bytes|
  body << bytes
  puts " -- stream.closed? #{stream.closed?}"
end

stream.on(:close) do
  puts '-' * 80
  puts " -- stream.closed? #{stream.closed?}"
  puts headers.inspect
  puts body.inspect
end
 <- {:length=>19, :type=>:headers, :flags=>[:end_headers], :stream=>1, :payload=>"\x88_\x90\x1Du\xD0b\r&=LMed\xFFu\xD8t\x9F"}
 -- stream.closed? false
 <- {:length=>12, :type=>:data, :flags=>[], :stream=>1, :payload=>"\x00\x00\x00\x00\a\n\x05reply"}
 -- stream.closed? false
 <- {:length=>12, :type=>:headers, :flags=>[:end_stream, :end_headers], :stream=>1, :payload=>"@\x88\x9A\xCA\xC8\xB2\x124\xDA\x8F\x010"}
 -- stream.closed? false
--------------------------------------------------------------------------------
 -- stream.closed? true
[[":status", "200"], ["content-type", "application/grpc+proto"], ["grpc-status", "0"]]
"\u0000\u0000\u0000\u0000\a\n\u0005reply"
xiejiangzhi commented 5 years ago

If I cannot get the flag of current frame from on(:headers) and on(:data), only two choices for me. one is to put headers and data to a buffer, and send them when stream close, the other is to handle the on(:frame_received) and forward headers/data