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

HTTP/2 features required for gRPC #1191

Closed
willemdj opened this issue Jul 26, 2017 · 45 comments
Closed

HTTP/2 features required for gRPC #1191

willemdj opened this issue Jul 26, 2017 · 45 comments

Comments

@willemdj
Copy link

It would be great if Cowboy could be used to support gRPC. The fact that there is now support for flow control in the HTTP/2 implementation brings that a lot closer, but it is still not possible to support gRPC without some patches.

As far as I can see, there are still some things missing in the HTTP/2 support:

  • not possible to send trailers? (issue http trailers support #1020)
  • not possible to omit the "date"and "server" header.
    Issue Option to disable "server" header #680 says that this can be addressed using hooks, but I am unable to
    find this in the 2.0 documentation (google gives me
    https://ninenines.eu/docs/en/cowboy/2.0/guide/hooks/, which results in a 404
    error).
  • not possible (except with a complicated workaround) to access the peer
    certificate (issue Ability to access the client SSL Certificate in V2.0 #1147)
  • not possible to influence the order of headers. gRPC requires that headers
    are in a certain order. The approach used by Cowboy (headers in maps) cannot
    guarantee that (although it seems to work in practice, but I assume that
    that is due to an implementation choice in the support for maps which cannot
    be relied upon).
  • not possible to manage flow control? (Not strictly required for HTTP/2, as
    long as there is something to avoid that the traffic is stalled, which is
    there now).

Any thoughts on this?

@essen
Copy link
Member

essen commented Jul 26, 2017

Trailers will be added in a point release after 2.0, I had to cut it from the plans in order to move the release forward.

All headers can be modified via a stream handler (except HTTP/1.1 headers connection and transfer-encoding, currently).

Peer certificate was also cut in order to release 2.0. It'll be added back in a point release soon after.

Order of headers... That's a weird requirement, do you have a link to the relevant part of the gRPC spec?

Flow control can be managed directly via stream handlers also, via the {flow, N} command.

@willemdj
Copy link
Author

I'll have a look at the stream handlers, thanks.

Regarding the order of the headers, see https://grpc.io/docs/guides/wire.html:

HTTP2 requires that reserved headers, ones starting with “:” appear before all other headers. Additionally implementations should send Timeout immediately after the reserved headers and they should send the Call-Definition headers before sending Custom-Metadata.

@essen
Copy link
Member

essen commented Jul 26, 2017

That... doesn't make a lot of sense. HTTP/2 headers are compressed, so I doubt you'll see too many implementations mixing decompression and other operations. My gut feeling is that you'll have no issues with the current implementation.

@willemdj
Copy link
Author

Them being compressed doesn't mean that they don't have a specific order, I think?

But you are right, so far I did not run into any issues. It is a "should" requirement anyway, so clients must be able to deal with cases where the order is different.

@essen
Copy link
Member

essen commented Jul 26, 2017

Also the reason the pseudo-header fields are properly ordered right now seems to come from the fact that maps:to_list currently returns a list ordered by key. This is not guaranteed though and we should probably add a lists:keysort to enforce it at some point.

@essen
Copy link
Member

essen commented Jul 26, 2017

Right this doesn't mean there is no order, there is an order. Ultimately i would like to have a way to tweak that order because even HTTP/1.1 has a recommended order, but this will have to take the form of a callback defining weights for the different header names or something, and is not quite a priority at the moment. :-)

@willemdj
Copy link
Author

I have been looking at the 'flow' command.

  • Am I right that there is no automatic/default flow control? Only via the 'flow' command?

  • How does it work with the connection window (as opposed to the stream window)? Am I right that it is only possible to increase both (with the same amount)? So the connection window will always be equal to the sum of the stream windows?

  • Is there a way to get access to the current size of the windows (both local and remote, stream and connection)?

@essen
Copy link
Member

essen commented Jul 26, 2017

It just informs Cowboy that you want to receive this much more data on this stream (and by extension on the connection). What does gRPC require?

@willemdj
Copy link
Author

gRPC doesn't require anything specifically, just that HTTP/2 is used.

But as a programmer who implements gRPC services, it would be good to have some features:

When receiving requests:

  • The option to have automatic flow control with some sane defaults or parameters, which ensure that there is always room for the next request (without sending an update for both windows for every request) - so essentially not having to care about flow control at all.

  • the option to actually manage the window explicitly, if I want to be able to throttle the traffic. For such a case it would be good to have access to the current window size.

When sending responses:

  • to know how much I am still allowed to send (or at least to get some signal if I try to send something that cannot be sent straight away).

Having a strong link between the Connection window and the Stream window makes the Connection window pointless. In theory there could be scenarios where you want to stop getting some kinds of messages, but allow others.

@essen
Copy link
Member

essen commented Jul 26, 2017

Your last comment is already possible I think, Cowboy just ensures the connection window is at least as big as all current stream windows. If stream A's window is at 0 and stream B at 1000, the connection window will be at least at 1000 and only stream B will receive data.

The only thing you can't do with the current strategy is have the connection window lower than the sum of all stream windows, but that's something that will require a different interface specific to HTTP/2 (just like we'll need an interface for priority handling specific to HTTP/2).

@willemdj
Copy link
Author

Yes, you are right about that.

@willemdj
Copy link
Author

I ran into a problem due to the fact that there is no warning when a message is queued because of the flow control: when I send a trailer this will go through immediately, while there may still be data messages in the queue if no WINDOW_UPDATE message was received (yet).

I realize that the sending of trailers is something I added myself in my own fork, but I think it illustrates the point that it would be good to have some kind of indication when things cannot be sent immediately.

Looking at my original message, I think that there are two things that are not in other tickets:

  1. the order of the headers, but we concluded that it is not really important
  2. the flow control limitations, which I still see as a problem.

Should I create a new ticket for the second point, so that we can close this one?

@essen
Copy link
Member

essen commented Jul 29, 2017

Well if you add support for trailers you're supposed to queue them too. Simple as that. :-)

@willemdj
Copy link
Author

willemdj commented Jul 30, 2017

You are right, if there is queuing then the trailers should also be queued.

The alternative is not to have any queuing. I would prefer that: to get some indication back that a message cannot be sent, rather than it being queued.

Or, if that is not feasible, to be able to retrieve some information about the current window size of a stream, so that I can decide for myself to not send any data. I could even do some bookkeeping to avoid having to call that function for every message, but I cannot do it completely outside cowboy because I don't see the window updates.

In any case it would be helpful for me to know the approach that you are most likely to take, so that I can eventually get rid of my fork without having to do too much rework...

@essen
Copy link
Member

essen commented Jul 30, 2017

I see what you mean. But yes, this would most likely require something HTTP/2 specific, since HTTP/1.1 doesn't have windows, and that will be worked on sometimes after 2.0. Please do experiment and provide your findings as I don't really know how to go about it yet.

@essen
Copy link
Member

essen commented Nov 4, 2017

Hello, as 2.1 is basically ready and 2.2 is the gRPC features list, can you confirm whether something other than trailers is required? If it's only trailers, I might even be able to squeeze it in 2.1 next week.

@willemdj
Copy link
Author

willemdj commented Nov 5, 2017

Yes, I think with the trailers (and the peer certificate, which you took are of already), it would be possible to support gRPC without the need for a fork. That would be great!

@essen
Copy link
Member

essen commented Nov 5, 2017

OK then.

Can you expand on this comment btw? Bluehouse-Technology/grpc#9 (comment) I thought everything was fine the last time we discussed it.

@willemdj
Copy link
Author

willemdj commented Nov 6, 2017

I certainly don't think it is a major issue. What it refers to is what is mentioned above in the messages of 30 July.

If the server streams messages to the client, they may hit the window size. At that moment they are queued, which may be fine in many cases, but it would be nice to have some information about such a case, so that the implementation can do something about it. Reduce the speed, apply some back pressure, ...

At that moment you suggested to experiment. I cannot say that I did that, but I did think about it and had a closer look at the implementation as it was at that time, and I could not come up with a quick and simple solution.

@essen
Copy link
Member

essen commented Nov 6, 2017

A stream handler can easily keep track of windows and implement back pressure. I'll walk you through it if necessary.

@willemdj
Copy link
Author

willemdj commented Nov 6, 2017 via email

@essen
Copy link
Member

essen commented Nov 6, 2017

I think you are correct in that it's currently not possible, but all we are missing is an event when the client increased the window. That's trivial and harmless to add to Cowboy.

Then once you have that, all you need is a stream handler that keeps track of this window (when you send data, decrease, when you receive this event, increase) and sends a message to the request process telling it to block/unblock depending on the window. I'm assuming here that you would use a loop handler that can easily process messages, or have a wrapper around cowboy_req for the data sending functions that would check if it needs to block before it sends more.

@essen
Copy link
Member

essen commented Nov 6, 2017

I can see Cowboy getting its own mechanism for this too, I'm sure some people will want this feature.

@willemdj
Copy link
Author

willemdj commented Nov 6, 2017

Yes, your are right. I found the notes that I made for myself at that time, which include this "to do" item (for myself):

  • grpc:send will return {ok, Stream} or {{error, flow_control}, Stream}.
    Examples and docs need to be modified. Cowboy stream handler (cowboy_http2) keeps track
    of window. When it is depleted, sending a messages fails. (new "maybe_send()" function). A new
    cowboy_req function is created to register a callback function to be called when a window update is
    received.

It is similar to what you describe above, I think. I guess that I was looking originally for a solution that would not have to duplicate the functionality to keep track of the window. Anyway, I don't think it is very important - getting a grpc server working that supports the basics of grpc without the need for a cowboy clone will be a very good start.

@essen
Copy link
Member

essen commented Nov 6, 2017

Yep. I'm writing tests for trailers now. If I don't get distracted too much this'll make it into 2.1.

@essen
Copy link
Member

essen commented Nov 6, 2017

Is it only responses that need trailer for gRPC?

@willemdj
Copy link
Author

willemdj commented Nov 6, 2017 via email

@essen
Copy link
Member

essen commented Nov 6, 2017

Yeah just wanted to make sure. :-)

@essen
Copy link
Member

essen commented Nov 6, 2017

OK I got some sort of trailer support for both HTTP/1.1 and HTTP/2. I think I will release Cowboy 2.1 without them though, but commit this right after that. Then you can start your work using them and we can iron out issues together. I will also feel better if I can write a few more tests before making it official.

@willemdj
Copy link
Author

willemdj commented Nov 6, 2017 via email

@essen
Copy link
Member

essen commented Nov 15, 2017

Trailers are in! 39baed6

Please start working on this, I will add a few tests when I get more time and then once I got the OK from you will release 2.2. Thanks!

@willemdj
Copy link
Author

I believe that trailers are not queued? This is causing one of my tests to fail. See also above: "if you add support for trailers you're supposed to queue them too." :)

Since the implementation cannot know that things have been queued, it may now send a trailer that goes through to the client, while some DATA messages are still in the queue.

@essen
Copy link
Member

essen commented Nov 19, 2017

Well that was the intent, but... whoops I guess. Fix should be simple enough, should just look at the queue, if empty send immediately, otherwise queue.

@essen
Copy link
Member

essen commented Nov 19, 2017

Apparently when trailers aren't supported we still send the trailer header in the response, so I'll be fixing that tomorrow also.

@essen
Copy link
Member

essen commented Nov 20, 2017

There's a bug when compression and trailers are both used also.

@essen
Copy link
Member

essen commented Nov 20, 2017

Most recent commit has fixes for both the compression bug and the queue bug along with a test for it. I haven't done the removal of the trailer header when trailers are not supported, I'm not yet convinced it's the right thing to do.

Please try against your tests and feedback!

@willemdj
Copy link
Author

Now all my tests pass, great!

@smorin
Copy link

smorin commented Nov 23, 2017

@willemdj are you working on a public grpc erlang or elixir implementation?

@willemdj
Copy link
Author

@essen
Copy link
Member

essen commented Nov 23, 2017

Can you add it to Erlang.mk packages @willemdj ? https://erlang.mk/guide/contributing.html#_packages

Cheers

@tony612
Copy link
Contributor

tony612 commented Nov 23, 2017

@smorin I'm writing an Elixir implementation https://github.com/tony612/grpc-elixir

@essen Is there a way to read_body before IsFin=fin via cowboy_stream_h? So that I can implement client side streaming grpc services(servers can handle streaming messages one by one).

@essen
Copy link
Member

essen commented Nov 23, 2017

You can already read_body before IsFin=fin so I'm afraid you're going to be a bit more specific. cowboy_stream_h has a behavior that matches that of the cowboy_req:read_body function. If what you need is for data to be forwarded to the request process as soon as it arrives, then you either need to write a stream handler to do it, or wait for the corresponding feature in Cowboy (this is planned but no dates yet).

@tony612
Copy link
Contributor

tony612 commented Nov 23, 2017

@essen Yes, I expect to read current buffer or get the data as soon as it comes as you said.

@essen
Copy link
Member

essen commented Nov 23, 2017

Then yes you probably need to write some custom stream handler for now, mimicking what cowboy_stream_h does except without the buffer; otherwise try tweaking the read_body options to make it send after it receives a very small amount of data (but that won't be asynchronous).

Would love to see what you come up with as we'll need something similar in Cowboy to allow streaming request bodies via loop handlers.

@essen
Copy link
Member

essen commented Dec 11, 2017

I've documented the new feature so closing this, thanks!

@essen essen closed this as completed Dec 11, 2017
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

No branches or pull requests

4 participants